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.

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