You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/31 10:42:03 UTC
[GitHub] [kafka] divijvaidya opened a new pull request, #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
divijvaidya opened a new pull request, #12230:
URL: https://github.com/apache/kafka/pull/12230
InvocationTargetException always wraps an underlying cause. It makes sense to catch it as soon as possible and only propagate the underlying cause.
# Change
1. Replace "legacy" [1] with `getCause()`
2. Propagate the cause after catching InvocationTargetException
[1] "legacy as per the docs" https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/InvocationTargetException.html
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r903680274
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
Yes. This method is only used in trogdor today and there is no specific handling of ClassNotFoundException exception.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12230:
URL: https://github.com/apache/kafka/pull/12230#issuecomment-1156355538
@dengziming @showuon please review this small change.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r955866446
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
True, but I would disagree that it is a downside.
In my opinion, a caller should be handling the underlying cause because the `InvocationTargetException` is being thrown from business logic inside the constructor. If the caller catches a generic `ReflectiveOperationException` it doesn't get to know (without inspecting details) whether the exception is due to a "reflection op error" such as method not found, class not found etc. or whether it is an exception thrown by the business logic itself. I believe that propagating the underlying business logic exception is better in this scenario.
Having said that, I don't have a strong opinion on this one as it's a minor improvement that will only impact code debuggability when looking at exception traces. Happy to revert if you think otherwise.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r956053947
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
The caller may be itself generic code. For example, see what happened here:
https://github.com/apache/kafka/pull/12230/files#diff-ac079c35d502aee90e8bbfa3b71cd9deda5ad4ffb49364fcc23a9278a848241cR79
The method now throws `Throwable`. It was pretty hard for me to say whether we needed to update some code to handle other exceptions. Is it clear to you?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mdedetrich commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r955962277
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
I also agree with @divijvaidya here, having a string distinction between business logic and reflection based exceptions is ideal and in my opinion over the long term is less brittle (since if you catch business logic exceptions that is unlikely to change)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r952378530
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
Wouldn't a `ClassNotFoundException` be wrong in this context and give a wrong impression to user? I am saying this because it's not a problem with Class not being on the JVM's classpath, instead `InvocationTargetException` is thrown when we have found the Class and invoking the constructor is throwing an exception in the line `constructor.newInstance(args`
I am happy to replace KafkaException with other generic RuntimeException that you would suggest but `ClassNotFoundException` appears inappropriate to me.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
showuon merged PR #12230:
URL: https://github.com/apache/kafka/pull/12230
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r900163290
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
Have we checked that changing this exception doesn't cause an issue?
##########
clients/src/test/java/org/apache/kafka/common/KafkaFutureTest.java:
##########
@@ -637,7 +637,12 @@ public void testLeakCompletableFuture() throws NoSuchMethodException, Invocation
// Check the CF from a minimal CompletionStage doesn't cause completion of the original KafkaFuture
Method minimal = CompletableFuture.class.getDeclaredMethod("minimalCompletionStage");
- CompletionStage<String> cs = (CompletionStage<String>) minimal.invoke(comfut);
+ CompletionStage<String> cs = null;
+ try {
+ cs = (CompletionStage<String>) minimal.invoke(comfut);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
Review Comment:
Maybe we should have a helper `invoke` method since we do this many times in this test?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12230:
URL: https://github.com/apache/kafka/pull/12230#issuecomment-1203693391
@mimaison kindly review when you get a chance.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12230:
URL: https://github.com/apache/kafka/pull/12230#issuecomment-1205399727
@showuon requesting for your code review time on this one. We already have two approvals from non-committers.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r952370012
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
Could we throw `ClassNotFoundException` like before, and pass `e.getCause()` to it instead? I think throwing `ClassNotFoundException` for consistency is better.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r903718578
##########
clients/src/test/java/org/apache/kafka/common/KafkaFutureTest.java:
##########
@@ -637,7 +637,12 @@ public void testLeakCompletableFuture() throws NoSuchMethodException, Invocation
// Check the CF from a minimal CompletionStage doesn't cause completion of the original KafkaFuture
Method minimal = CompletableFuture.class.getDeclaredMethod("minimalCompletionStage");
- CompletionStage<String> cs = (CompletionStage<String>) minimal.invoke(comfut);
+ CompletionStage<String> cs = null;
+ try {
+ cs = (CompletionStage<String>) minimal.invoke(comfut);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
Review Comment:
Thanks for your suggestion. I have made the changes that you suggested in the latest commit.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12230:
URL: https://github.com/apache/kafka/pull/12230#issuecomment-1223768524
The failing tests for JDK 11 are unrelated to the code changes.
```
Build / JDK 11 and Scala 2.13 / testLargeAssignmentAndGroupWithNonEqualSubscription() – org.apache.kafka.clients.consumer.StickyAssignorTest
1m 3s
Build / JDK 11 and Scala 2.13 / testPrepareJoinAndRejoinAfterFailedRebalance() – org.apache.kafka.clients.consumer.internals.EagerConsumerCoordinatorTest
<1s
Build / JDK 11 and Scala 2.13 / testTimeouts() – org.apache.kafka.controller.QuorumControllerTest
```
@ijuma @mimaison requesting your your time to review this. Please note that we have this triaged with two non-committers already.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mdedetrich commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r955962277
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
I also agree with @divijvaidya here, having a strong distinction between business logic and reflection based exceptions is ideal and in my opinion over the long term is less brittle (since if you catch business logic exceptions that is unlikely to change)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r952380866
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
Good point! You're right! When entering here, the constructor is already found and called, just there's exception thrown. Let's keep `KafkaException` like you did. Thank you.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r955599249
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
One downside of this approach is that one cannot catch ReflectiveException to catch all expected exceptions.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r956552230
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
@ijuma , thanks for the explanation. Make sense to me.
@divijvaidya I think it's better we revert it. 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12230: MINOR: Catch InvocationTargetException explicitly and propagate underlying cause
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12230:
URL: https://github.com/apache/kafka/pull/12230#discussion_r957021263
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -463,8 +463,7 @@ public static <T> T newParameterizedInstance(String className, Object... params)
throw new ClassNotFoundException(String.format("Unable to access " +
"constructor of %s", className), e);
} catch (InvocationTargetException e) {
- throw new ClassNotFoundException(String.format("Unable to invoke " +
- "constructor of %s", className), e);
+ throw new KafkaException(String.format("The constructor of %s threw an exception", className), e.getCause());
Review Comment:
@showuon sure, let's do it. I understand the POV where @ijuma is coming from.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org