You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2026/05/29 14:25:32 UTC
[PR] Common: Handle ExceptionInInitializerError & NoClassDefFoundError during optional Dyn* class loading [iceberg]
nastra opened a new pull request, #16611:
URL: https://github.com/apache/iceberg/pull/16611
`Class.forName` can throw `NoClassDefFoundError` or `ExceptionInInitializerError` when a class is found but a transitive dependency is missing or its static initializer fails. This should be treated the same as `ClassNotFoundException` for optional implementation probing.
The initial version was created by Claude and tests were manually adjusted
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3460927821
##########
core/src/main/java/org/apache/iceberg/formats/FormatModelRegistry.java:
##########
@@ -205,12 +205,13 @@ private static void registerSupportedFormats() {
for (String classToRegister : CLASSES_TO_REGISTER) {
try {
DynMethods.builder("register").impl(classToRegister).buildStaticChecked().invoke();
- } catch (NoSuchMethodException e) {
+ } catch (NoSuchMethodException | NoClassDefFoundError | ExceptionInInitializerError e) {
// failing to register a factory is normal and does not require a stack trace
+ String message = e.getCause() != null ? e.getCause().getMessage() : e.getMessage();
LOG.info(
"Unable to call register for ({}). Check for missing jars on the classpath: {}",
classToRegister,
- e.getMessage());
+ message);
Review Comment:
It's not clear to me that we can still see which underlying cause triggered the failure. It's important to know whether you have a class mismatch or you're missing a class.
--
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] Common, Core: Handle ExceptionInInitializerError & NoClassDefFoundError during optional Dyn* class loading [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3374663075
##########
common/src/main/java/org/apache/iceberg/common/DynConstructors.java:
##########
@@ -144,7 +144,7 @@ public Builder impl(String className, Class<?>... types) {
try {
Class<?> targetClass = classForName(className);
impl(targetClass, types);
- } catch (NoClassDefFoundError | ClassNotFoundException e) {
+ } catch (NoClassDefFoundError | ClassNotFoundException | ExceptionInInitializerError e) {
Review Comment:
Please remove changes to this class. I don't think that this is necessary to fix the bug and this is an over-broad change to the contract of our reflection classes that are used in a lot of places.
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #16611:
URL: https://github.com/apache/iceberg/pull/16611
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3381171248
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
reproducing the original issue in a unit test without mocking out a bunch of things is a bit difficult because we'd have to simulate that parquet/orc isn't on the classpath. Also I didn't want to provide a fix without having a test that at least shows how the issue can happen in practice. So I figured we might want to just have a try/catch block that essentially does what's being handled in the affected classes.
I'm open to other ideas though. Using a package-private `classesToRegister()` might work for `FormatModelRegistry` but not for `InternalData`.
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3461109931
##########
core/src/main/java/org/apache/iceberg/formats/FormatModelRegistry.java:
##########
@@ -205,12 +205,13 @@ private static void registerSupportedFormats() {
for (String classToRegister : CLASSES_TO_REGISTER) {
try {
DynMethods.builder("register").impl(classToRegister).buildStaticChecked().invoke();
- } catch (NoSuchMethodException e) {
+ } catch (NoSuchMethodException | NoClassDefFoundError | ExceptionInInitializerError e) {
// failing to register a factory is normal and does not require a stack trace
+ String message = e.getCause() != null ? e.getCause().getMessage() : e.getMessage();
LOG.info(
"Unable to call register for ({}). Check for missing jars on the classpath: {}",
classToRegister,
- e.getMessage());
+ message);
Review Comment:
I agree. We should handle these errors, but distinguish between the cases.
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3501996141
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
With the latest updates, the tests do seem detached from what's going on in `InternalData` or `FormatModelRegistry`. It's particularly difficult to test this because it's being triggered from a static block.
The only way I can think to test this is to create an `IsolatingClassLoader` that just loads that (or a subset) of classes to trigger this behavior in a way that we can 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: 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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#issuecomment-4875876599
I'll leave it open for a few more days in case @pvary or @rdblue have any further comments
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3460932831
##########
core/src/main/java/org/apache/iceberg/InternalData.java:
##########
@@ -65,9 +65,10 @@ private static void registerSupportedFormats() {
registerParquet.invoke();
- } catch (NoSuchMethodException e) {
+ } catch (NoSuchMethodException | NoClassDefFoundError | ExceptionInInitializerError e) {
// failing to load Parquet is normal and does not require a stack trace
- LOG.info("Unable to register Parquet for metadata files: {}", e.getMessage());
+ String message = e.getCause() != null ? e.getCause().getMessage() : e.getMessage();
+ LOG.info("Unable to register Parquet for metadata files: {}", message);
Review Comment:
Same thing here. I think we should include what exception was thrown.
--
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] Common, Core: Handle ExceptionInInitializerError & NoClassDefFoundError during optional Dyn* class loading [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3325191085
##########
common/src/main/java/org/apache/iceberg/common/DynClasses.java:
##########
@@ -66,7 +66,7 @@ public Builder impl(String className) {
try {
this.foundClass = Class.forName(className, true, loader);
- } catch (ClassNotFoundException e) {
+ } catch (ClassNotFoundException | NoClassDefFoundError | ExceptionInInitializerError e) {
// not the right implementation
Review Comment:
```suggestion
// cannot load this implementation
```
##########
core/src/main/java/org/apache/iceberg/formats/FormatModelRegistry.java:
##########
Review Comment:
ExceptionInInitializerError.getMessage() would return null. One way to fix it is to check if `e.getCause()` is not null and if so use its message.
##########
common/src/main/java/org/apache/iceberg/common/DynConstructors.java:
##########
@@ -144,7 +144,7 @@ public Builder impl(String className, Class<?>... types) {
try {
Class<?> targetClass = classForName(className);
impl(targetClass, types);
- } catch (NoClassDefFoundError | ClassNotFoundException e) {
+ } catch (NoClassDefFoundError | ClassNotFoundException | ExceptionInInitializerError e) {
// cannot load this implementation
Review Comment:
comment is stale (same fix as above). Same applies for below
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3466082014
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
This test essentially **copies** the relevant logic from the production code and verifies the outcome of those calls. The problem is that if we change anything in `FormatModelRegistry` or `InternalData`, the test will silently become stale because it **copies** the implementation rather than validating its behavior independently.
While modifying production code purely for testability is not always the most elegant approach, I think extracting testable methods is a reasonable and widely accepted trade-off. It gives us a more robust test that remains aligned with the actual implementation instead of relying on copied logic that will drift over 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] Common: Handle ExceptionInInitializerError & NoClassDefFoundError during optional Dyn* class loading [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3324992815
##########
core/src/main/java/org/apache/iceberg/formats/FormatModelRegistry.java:
##########
@@ -205,7 +205,7 @@ private static void registerSupportedFormats() {
for (String classToRegister : CLASSES_TO_REGISTER) {
try {
DynMethods.builder("register").impl(classToRegister).buildStaticChecked().invoke();
- } catch (NoSuchMethodException e) {
+ } catch (NoSuchMethodException | NoClassDefFoundError | ExceptionInInitializerError e) {
Review Comment:
Note that callers must still handle invoke-time failures, hence why I'm adding it here and in `InternalData`. I've also added tests to `TestDynMethods` that verify 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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3503984111
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
> What if we create a 'register(String)' method both in InternalData and FormatModelRegistry?
We can call this from the prouction code and from the test too.
@pvary @danielcweeks I've opted for this approach but didn't make `register()` package-private for testing and rather went for the reflection-based approach so that `register()` stays private. PTAL
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3465070011
##########
core/src/main/java/org/apache/iceberg/formats/FormatModelRegistry.java:
##########
@@ -205,12 +205,13 @@ private static void registerSupportedFormats() {
for (String classToRegister : CLASSES_TO_REGISTER) {
try {
DynMethods.builder("register").impl(classToRegister).buildStaticChecked().invoke();
- } catch (NoSuchMethodException e) {
+ } catch (NoSuchMethodException | NoClassDefFoundError | ExceptionInInitializerError e) {
// failing to register a factory is normal and does not require a stack trace
+ String message = e.getCause() != null ? e.getCause().getMessage() : e.getMessage();
LOG.info(
"Unable to call register for ({}). Check for missing jars on the classpath: {}",
classToRegister,
- e.getMessage());
+ message);
Review Comment:
makes sense, I've updated this to log the entire cause as a string instead of just the error msg
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3465011364
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
I'm not sure we want to introduce additional production code just to make this better testable. Let's see what @danielcweeks and @rdblue think about testing 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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3380939836
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
These tests are a bit awkward, and hard to understand what they are actually test. Shall we create a package private `classesToRegister()` method and override it in the tests?
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#discussion_r3386976971
##########
core/src/test/java/org/apache/iceberg/formats/TestFormatModelRegistry.java:
##########
@@ -86,6 +88,77 @@ void testFailingReRegistrations() {
.hasMessageContaining("Cannot register class");
}
+ @Test
+ void registerDoesNotFailWhenClassThrowsNoClassDefFoundOnInvoke() {
Review Comment:
What if we create a 'register(String)' method both in InternalData and FormatModelRegistry?
We can call this from the prouction code and from the test too.
--
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] Core: Handle ExceptionInInitializerError & NoClassDefFoundError invoke-time failures in DynMethods callers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #16611:
URL: https://github.com/apache/iceberg/pull/16611#issuecomment-4891138197
thanks for the reviews everyone!
--
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