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/12 17:55:43 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
kbendick opened a new pull request, #5259:
URL: https://github.com/apache/iceberg/pull/5259
The `runSafely` function gives an error prone warning for throwing from a finally block.
Originally, we fixed this via suppression in https://github.com/apache/iceberg/pull/5190, but I noticed this function was unused and so chose to deprecate it instead in https://github.com/apache/iceberg/pull/5205.
@rdblue wanted to keep the function, as its tested and could be useful in the future.
So this PR:
- fixes the issue discovered in #5190 of throwing `failure` when it's null
- attempts to throw `failure` after adding `e` as a suppressed error when it's non-null
- adds the `@Finally` suppression to remove the error prone log from the build.
Warning that is logged on build:
```
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] rdblue commented on pull request #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#issuecomment-1182579344
Thanks, @kbendick!
--
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 a diff in pull request #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#discussion_r919322963
##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -113,15 +114,20 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
try {
finallyBlock.run();
} catch (Exception e) {
- LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
+ LOG.warn("Suppressing failure in finally block", e);
failure.addSuppressed(e);
+ tryThrowAs(failure, e1Class);
Review Comment:
In this case, `failure` is already thrown because it is thrown in the `catch` block. `tryThrowAs` needs to be removed here. The logic was correct before, except for the wrong exception in the next block.
--
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 #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#discussion_r919444526
##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -113,15 +114,20 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
try {
finallyBlock.run();
} catch (Exception e) {
- LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
+ LOG.warn("Suppressing failure in finally block", e);
failure.addSuppressed(e);
+ tryThrowAs(failure, e1Class);
Review Comment:
Oh actually yeah I understand. The `failure` would have already been thrown, just the finally block gets run and we add one more suppression to it. So we don't need to throw it again. Thanks for the explanation.
--
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 #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5259:
URL: https://github.com/apache/iceberg/pull/5259
--
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 #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#discussion_r919255482
##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -113,15 +114,20 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
try {
finallyBlock.run();
} catch (Exception e) {
- LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
+ LOG.warn("Suppressing failure in finally block", e);
Review Comment:
I moved this log because we're not suppressing `e` in the other block.
--
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 #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#discussion_r919443059
##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -113,15 +114,20 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
try {
finallyBlock.run();
} catch (Exception e) {
- LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
+ LOG.warn("Suppressing failure in finally block", e);
failure.addSuppressed(e);
+ tryThrowAs(failure, e1Class);
Review Comment:
TIL. Will remove. Thanks guys.
--
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 a diff in pull request #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#discussion_r919294729
##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -113,15 +114,20 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
try {
finallyBlock.run();
} catch (Exception e) {
- LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
+ LOG.warn("Suppressing failure in finally block", e);
failure.addSuppressed(e);
+ tryThrowAs(failure, e1Class);
Review Comment:
This is probably not necessary and actually goes against the ErrorProne guideline of not throwing from `finally`.
The only way `failure` can be non-null here is if it was thrown from the main `try` block, in which case the generated bytecode will ensure it is re-thrown when `finally` completes.
--
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 a diff in pull request #5259: [API] Fix throwing null exception and add error prone suppression to runSafely functions
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5259:
URL: https://github.com/apache/iceberg/pull/5259#discussion_r919294729
##########
api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:
##########
@@ -113,15 +114,20 @@ public static <R, E1 extends Exception, E2 extends Exception, E3 extends Excepti
try {
finallyBlock.run();
} catch (Exception e) {
- LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
+ LOG.warn("Suppressing failure in finally block", e);
failure.addSuppressed(e);
+ tryThrowAs(failure, e1Class);
Review Comment:
This is probably not necessary and actually goes against the ErrorProne guideline of not throwing from `finally`.
The only way `failure` can be non-null here is if it was thrown from the main `try` block, in which case the generated bytecode will ensure it is re-thrown when `finally` completes.
edit: the same applies to lines 126-130.
--
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