Repository navigation
Core: Broaden exception handling in writer clean up logic - #12863
amogh-jahagirdar merged 1 commit into
Conversation
c1aae7c to
64cee91
Compare
| } catch (UncheckedIOException e) { | ||
| // the file may not have been created, and it isn't worth failing the job to clean up, | ||
| // skip deleting | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
This catch is pretty broad what do you think about including some log message here?
There was a problem hiding this comment.
Thanks for the review! I was trying to keep the same behavior, but I don’t mind adding a log message if people think it would be helpful and not cause confusion.
There was a problem hiding this comment.
Thanks @xiaoxuandev @geruh . I generally agree with the change, since regardless of the type of failure here, it should not blow up the overall write being performed since this is just a best effort cleanup. I also agree with a warn log.
My only ask is we follow the same pattern like we do in other places and just use Tasks to express this.
Check out https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L485 , which essentially has the same best effort cleanup that we want.
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @xiaoxuandev , overall the premise of this change makes sense to me, just had a comment about using Tasks like we do in other places where best effort cleanup is performed
| } catch (UncheckedIOException e) { | ||
| // the file may not have been created, and it isn't worth failing the job to clean up, | ||
| // skip deleting | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
Thanks @xiaoxuandev @geruh . I generally agree with the change, since regardless of the type of failure here, it should not blow up the overall write being performed since this is just a best effort cleanup. I also agree with a warn log.
My only ask is we follow the same pattern like we do in other places and just use Tasks to express this.
Check out https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L485 , which essentially has the same best effort cleanup that we want.
64cee91 to
de9907d
Compare
|
Thanks @amogh-jahagirdar for the review! I’ve updated the change to follow the same cleanup approach, could you help take another look? |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @xiaoxuandev , just a nit but non-blocking. I'll leave it up until tomorrow morning, for others to review and then I'll go ahead and merge if there are no objections!
| .onFailure( | ||
| (file, exc) -> | ||
| LOG.warn( | ||
| "Failed to delete the empty file during writer clean up: {}", |
There was a problem hiding this comment.
Nit: Not sure the fact that it's an empty file is really useful in the log (maybe, as a user if you see the warn log that it's empty you're less concerned about how much dangling storage is there). I'd probably leave the "the empty" out but I'm also OK with it as it is since it's probably such a rare case that the log will be fired off.
There was a problem hiding this comment.
Or if we want to say "empty" to avoid user confusion on what's attempted to be deleted in the logs, I'd actually go a step further and say "uncommitted empty file". The uncommitted part is actually important to indicate to a user that we're not deleting anything that's part of their table.
There was a problem hiding this comment.
Good point! Calling it an "uncommitted empty file" makes it much clearer and avoids confusion, updated.
de9907d to
b571ced
Compare
|
Thanks @xiaoxuandev and thanks @geruh for reviewing, I'll go ahead and merge |
3 participants
Expanded the exception handling during writer cleanup to address scenarios where users may lack delete permissions, ensuring consistent behavior with uncommitted file deletion handling.