[GitHub] [iceberg] kbendick commented on a diff in pull request #5190: [API] Fix ErrorProne warnings in API module

[GitBox <gi...@apache.org>]

kbendick commented on code in PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#discussion_r914164348


##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -116,12 +116,17 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
           LOG.warn("Suppressing failure in finally block", e);
           if (failure != null) {
             failure.addSuppressed(e);
+            tryThrowAs(failure, e1Class);
+            tryThrowAs(failure, e2Class);
+            tryThrowAs(failure, e3Class);
+            tryThrowAs(failure, RuntimeException.class);
+            throw new RuntimeException("Unknown throwable", failure);

Review Comment:
   This seems like it will fix what the warning is about, that throwing from a finally block will swallow whatever is returned or thrown inside of the try block.
   
   So this repeats lines 105-109, where we attempt to throw `failure` after running the catch block. As the warning message from error prone says that it will get swallowed if we throw inside of the finally, but we did try to throw it inside of the catch block.
   
   Though I think this also changes the semantics of the function as intended. So maybe we should just add the suppression?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org