Don't throw from finally block #5222
Conversation
Any exception coming from the try-catch block will be swallowed and the user will only see the exception from the finally block. Note that `cleanupMetadataAndUnlock()` is being called from a `finally` block. It seems enough to just log the error but not throw an exception when metadata cleanup failed.
| } catch (RuntimeException e) { | ||
| LOG.error("Fail to cleanup metadata file at {}", newMetadataLocation, e); | ||
| throw e; | ||
| LOG.error("Failed to cleanup metadata file at {}", newMetadataLocation, e); |
There was a problem hiding this comment.
Would you consider adding e as a "suppressed" exception to any previous exception?
It's a bit more refactoring, but often having more information in the thrown exception is more useful than logs.
There was a problem hiding this comment.
that might be a good option. Let's see what others think
There was a problem hiding this comment.
I like this option as well. As we lose the CommitFailedException, other than the log... but the exception is the most likely thing to be seen.
But I'd personally wait a bit to see if we hear from somebody at AWS.
There was a problem hiding this comment.
I think this is fine. If it were easier, I'd opt to suppress the exception (maybe we should consider using runSafely for that) but since this is just warning that a file wasn't cleaned up, the important thing is not to throw.
4 participants
Any exception coming from the try-catch block will be swallowed and the user will only see the exception from the finally block.
Note that
cleanupMetadataAndUnlock()is being called from afinallyblock.It seems enough to just log the error but not throw an exception when
metadata cleanup failed.