wombatu-kun commented on code in PR #16793:
URL: https://github.com/apache/iceberg/pull/16793#discussion_r3407338452
##########
common/src/main/java/org/apache/iceberg/common/DynConstructors.java:
##########
@@ -222,7 +222,7 @@ public <C> Ctor<C> build() {
private Class<?> classForName(String className) throws
ClassNotFoundException {
try {
return Class.forName(className, true, loader);
- } catch (ClassNotFoundException e) {
+ } catch (ClassNotFoundException | NoClassDefFoundError e) {
Review Comment:
This re-introduces the change @rdblue asked to remove on #16611, where he
objected that widening the Dyn* catch is "an over-broad change to the contract
of our reflection classes that are used in a lot of places" and not necessary
to fix the bug (that thread is still unresolved there). #16611 instead handles
the failure in the specific callers (`FormatModelRegistry`, `InternalData`).
Worth linking that discussion here and making the case for changing the global
Dyn* contract versus the caller-side fix, since this turns any
`NoClassDefFoundError` from a present-but-unloadable class into "not found" for
every caller.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
