You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nk1506 (via GitHub)" <gi...@apache.org> on 2023/11/24 07:15:46 UTC
[PR] Remove redundant error propagation check. [iceberg]
nk1506 opened a new pull request, #9143:
URL: https://github.com/apache/iceberg/pull/9143
[Throwables.propagate](https://github.com/google/guava/blob/e5cc39c9e0bcca1347a937cb090321fb96de0e5c/guava/src/com/google/common/base/Throwables.java#L229) always translate to `RuntimeException`
Adding another check as `Throwables.propagateIfInstanceOf(e.getCause(), RuntimeException.class);` is redundant.
--
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
Re: [PR] Remove redundant error propagation check. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9143:
URL: https://github.com/apache/iceberg/pull/9143#issuecomment-2412570759
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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
Re: [PR] Remove redundant error propagation check. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9143:
URL: https://github.com/apache/iceberg/pull/9143#discussion_r1426541260
##########
common/src/main/java/org/apache/iceberg/common/DynConstructors.java:
##########
@@ -67,7 +66,6 @@ public C newInstance(Object... args) {
try {
return newInstanceChecked(args);
} catch (Exception e) {
- Throwables.propagateIfInstanceOf(e, RuntimeException.class);
Review Comment:
I do not think it is safe to remove.
The result is that the `RuntimeException`s will be wrapped in a new exception which will cause issues with error handling
--
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
Re: [PR] Remove redundant error propagation check. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9143:
URL: https://github.com/apache/iceberg/pull/9143#discussion_r1426541867
##########
common/src/main/java/org/apache/iceberg/common/DynMethods.java:
##########
@@ -62,7 +62,6 @@ public <R> R invokeChecked(Object target, Object... args) throws Exception {
} catch (InvocationTargetException e) {
Throwables.propagateIfInstanceOf(e.getCause(), Exception.class);
- Throwables.propagateIfInstanceOf(e.getCause(), RuntimeException.class);
Review Comment:
I think it is ok to remove this - see comment above
--
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
Re: [PR] Remove redundant error propagation check. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9143: Remove redundant error propagation check.
URL: https://github.com/apache/iceberg/pull/9143
--
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
Re: [PR] Remove redundant error propagation check. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9143:
URL: https://github.com/apache/iceberg/pull/9143#discussion_r1426539287
##########
common/src/main/java/org/apache/iceberg/common/DynConstructors.java:
##########
@@ -58,7 +58,6 @@ public C newInstanceChecked(Object... args) throws Exception {
throw e;
} catch (InvocationTargetException e) {
Throwables.propagateIfInstanceOf(e.getCause(), Exception.class);
- Throwables.propagateIfInstanceOf(e.getCause(), RuntimeException.class);
Review Comment:
I think this line is never reached as the previous line always catches even `RuntimeException`s, so it is safe to remove this.
--
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
Re: [PR] Remove redundant error propagation check. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9143:
URL: https://github.com/apache/iceberg/pull/9143#discussion_r1426542220
##########
common/src/main/java/org/apache/iceberg/common/DynMethods.java:
##########
@@ -71,7 +70,6 @@ public <R> R invoke(Object target, Object... args) {
try {
return this.invokeChecked(target, args);
} catch (Exception e) {
- Throwables.propagateIfInstanceOf(e, RuntimeException.class);
Review Comment:
I do not think we should remove this - see comment above
--
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