[API] Fix ErrorProne warnings in API module

[kbendick]

The number of error prone warnings has slowly gotten somewhat higher.

This PR fixes all of the relatively simple error prone warnings for the API module, which will have API / ABI compatibility guarantees come the 1.0 release (and so the JavaDocs should hopefully be stable from then on).

[kbendick]

The 1 remaining error prone warning in API module requires a bit more caution when / if we adjust it as this function in ExceptionUtil accepts generic code blocks.

iceberg/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:124: warning: [Finally] If you return or throw from a finally, then values returned or thrown from the try-catch block will be ignored. Consider using try-with-resources instead.
            throw new RuntimeException("Unknown exception in finally block", failure);
            ^
    (see https://errorprone.info/bugpattern/Finally)

[dimas-b]

Re: https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java#L124 it does look like the most recent exception (e in code) is lost.

Perhaps that line should do throw new RuntimeException("Unknown exception in finally block", e) (since failure is certainly null there).

[nastra]

I agree with @dimas-b's comment. Let's switch from failure to e in L124

[rdblue]

If there are multiple exceptions, then I think we should add the one that isn't thrown as a suppressed exception on the one that we do throw.

[kbendick]

Looks like this can be closed as these were fixed in: #5200

Thanks all!

[kbendick]

Looks like this can be closed as these were fixed in: #5200

Thanks all!

My mistake. These two were not fixed in #5200. I'll switch from failure to e as mentioned by @nastra.

In master, I still see the warnings this PR originally fixed.

[kbendick]

Switching from throwing e instead of finally still gives the exception message. But I agree that at line 124, we know that failure is null so we should probably throw e instead as it's likely non-null.

And we're trying to throw e in the 4 lines above 127 (as we're not suppressing there):

iceberg/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java

Lines 117 to 124 in d563d6d

	if (failure != null) {
	failure.addSuppressed(e);
	} else {
	tryThrowAs(e, e1Class);
	tryThrowAs(e, e2Class);
	tryThrowAs(e, e3Class);
	tryThrowAs(e, RuntimeException.class);
	throw new RuntimeException("Unknown exception in finally block", failure);

So it looks like we need to change to throwing e at line 127 (where we've tried throwing e as several different types in the preceding lines) and then also add the suppression. WDYT?

[kbendick]

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?

[kbendick]

If there are multiple exceptions, then I think we should add the one that isn't thrown as a suppressed exception on the one that we do throw.

This seems to happen already on line 118, but we're not rethrowing it from within the finally block (which is what the warning message is mentioning if I'm not mistaken): I've added rethrowing failure when it's non-null to this PR.

iceberg/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java

Lines 105 to 118 in d563d6d

	tryThrowAs(failure, e1Class);
	tryThrowAs(failure, e2Class);
	tryThrowAs(failure, e3Class);
	tryThrowAs(failure, RuntimeException.class);
	throw new RuntimeException("Unknown throwable", failure);
	} finally {
	if (finallyBlock != null) {
	try {
	finallyBlock.run();
	} catch (Exception e) {
	LOG.warn("Suppressing failure in finally block", e);
	if (failure != null) {
	failure.addSuppressed(e);

[kbendick]

I just noticed that none of these runSafely functions are used anywhere other than TestExceptionUtil.

Maybe we ought to either remove them or mark them as deprecated?

[kbendick]

I reverted the changes to ExceptionUtils and opened a PR to deprecate the unused functions that are giving the warnings here #5205