MINOR: Catch InvocationTargetException explicitly and propagate underlying cause #12230

divijvaidya · 2022-05-31T10:42:01Z

InvocationTargetException always wraps an underlying cause. It makes sense to catch it as soon as possible and only propagate the underlying cause.

Change

Replace "legacy" [1] with getCause()
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

Kvicii

LGTM.

divijvaidya · 2022-06-15T11:30:37Z

@dengziming @showuon please review this small change.

ijuma

Thanks for the PR, left a couple of comments.

ijuma · 2022-06-17T14:14:30Z

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());


Have we checked that changing this exception doesn't cause an issue?

Yes. This method is only used in trogdor today and there is no specific handling of ClassNotFoundException exception.

Could we throw ClassNotFoundException like before, and pass e.getCause() to it instead? I think throwing ClassNotFoundException for consistency is better.

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.

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.

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.

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)

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?

@ijuma , thanks for the explanation. Make sense to me.
@divijvaidya I think it's better we revert it. WDYT?

@showuon sure, let's do it. I understand the POV where @ijuma is coming from.

clients/src/test/java/org/apache/kafka/common/KafkaFutureTest.java

divijvaidya · 2022-08-03T09:14:13Z

@mimaison kindly review when you get a chance.

mdedetrich

The changes look good from my end

divijvaidya · 2022-08-04T15:18:29Z

@showuon requesting for your code review time on this one. We already have two approvals from non-committers.

…ause

divijvaidya · 2022-08-23T08:57:33Z

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.

showuon

@divijvaidya , sorry for the delay. Overall LGTM, left one comment.

showuon · 2022-08-23T09:21:25Z

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());


Could we throw ClassNotFoundException like before, and pass e.getCause() to it instead? I think throwing ClassNotFoundException for consistency is better.

Kvicii approved these changes May 31, 2022

View reviewed changes

divijvaidya force-pushed the fixInvocationTarget branch from 46f1013 to 73afd1b Compare June 8, 2022 15:51

ijuma reviewed Jun 17, 2022

View reviewed changes

mdedetrich mentioned this pull request Aug 3, 2022

MINOR; Update scalafmt to latest version #12475

Merged

3 tasks

mdedetrich approved these changes Aug 3, 2022

View reviewed changes

Catch InvocationTargetException explicitly and propagate underlying c…

d7eab2d

…ause

divijvaidya force-pushed the fixInvocationTarget branch from 2d8dda6 to d7eab2d Compare August 22, 2022 14:07

showuon reviewed Aug 23, 2022

View reviewed changes

showuon approved these changes Aug 23, 2022

View reviewed changes

showuon merged commit 9aef992 into apache:trunk Aug 23, 2022

divijvaidya deleted the fixInvocationTarget branch August 23, 2022 09:44

MINOR: Catch InvocationTargetException explicitly and propagate underlying cause #12230

MINOR: Catch InvocationTargetException explicitly and propagate underlying cause #12230

Uh oh!

Conversation

divijvaidya commented May 31, 2022

Change

Uh oh!

Kvicii left a comment

Choose a reason for hiding this comment

Uh oh!

divijvaidya commented Jun 15, 2022

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdedetrich Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

divijvaidya commented Aug 3, 2022

Uh oh!

mdedetrich left a comment

Choose a reason for hiding this comment

Uh oh!

divijvaidya commented Aug 4, 2022

Uh oh!

divijvaidya commented Aug 23, 2022

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdedetrich Aug 26, 2022 •

edited

Loading