You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/04 02:55:46 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #5190: [API] Fix ErrorProne warnings in API module
kbendick opened a new pull request, #5190:
URL: https://github.com/apache/iceberg/pull/5190
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).
--
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
[GitHub] [iceberg] dimas-b commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
dimas-b commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1173790084
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).
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175460093
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?
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175469221
I reverted the changes to `ExceptionUtils` and opened a PR to deprecate the unused functions that are giving the warnings here https://github.com/apache/iceberg/pull/5205
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175434502
> 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 these warnings:
```
> Task :iceberg-api:compileJava
/Users/kylebendickson/repos/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)
/Users/kylebendickson/repos/iceberg/api/src/main/java/org/apache/iceberg/io/FileIO.java:65: warning: [MissingSummary] A summary line is required on public/protected Javadocs.
* @return the property map used to configure this FileIO
^
(see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
/Users/kylebendickson/repos/iceberg/api/src/main/java/org/apache/iceberg/io/CredentialSupplier.java:31: warning: [MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
* @return the credential string
^
(see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
Did you mean '*Returns the credential string.'?
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
3 warnings
```
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175459619
You know I just noticed that _none_ of these `runSafely` functions are used anywhere other than `TestExceptionUtil`.
--
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
[GitHub] [iceberg] rdblue merged pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5190:
URL: https://github.com/apache/iceberg/pull/5190
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175430799
Looks like this can be closed as these were fixed in: https://github.com/apache/iceberg/pull/5200
Thanks all!
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1173283625
The 1 remaining error prone warning in API module requires a bit more caution when / if we address it as quite a lot of code uses `ExceptionUtil`.
```
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)
```
--
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
[GitHub] [iceberg] kbendick commented on a diff in pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by 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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175456591
> 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):
https://github.com/apache/iceberg/blob/d563d6db0a56199306c82187027fb76e2bf41be2/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java#L118
--
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
[GitHub] [iceberg] nastra commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
nastra commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1173868079
I agree with @dimas-b's comment. Let's switch from `failure` to `e` in L124
--
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
[GitHub] [iceberg] kbendick commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175443108
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): https://github.com/apache/iceberg/blob/d563d6db0a56199306c82187027fb76e2bf41be2/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java#L117-L124
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?
--
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
[GitHub] [iceberg] rdblue commented on pull request #5190: [API] Fix ErrorProne warnings in API module
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5190:
URL: https://github.com/apache/iceberg/pull/5190#issuecomment-1175348374
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 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