wombatu-kun commented on code in PR #16793:
URL: https://github.com/apache/iceberg/pull/16793#discussion_r3407348089
##########
common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java:
##########
@@ -77,6 +77,69 @@ public void testLoaderFallback() throws Exception {
assertThat(ctor.newInstance()).isInstanceOf(MyClass.class);
}
+ @Test
+ public void implWithNoClassDefFoundError() throws Exception {
Review Comment:
Unlike the three sibling tests, this one passes whether or not the
`classForName` catch is widened (DynConstructors.java:225): `impl(String)` and
`hiddenImpl(String)` at DynConstructors.java:147 and :178 already catch
`NoClassDefFoundError`, so reverting the widening still records the problem and
`buildChecked()` still throws `NoSuchMethodException` here. The only behavior
the widening adds is the context-classloader fallback at
DynConstructors.java:227 retrying after a `NoClassDefFoundError`, and this test
never depends on it because `errorLoader`'s parent (the context loader) cannot
resolve the class either. To make this a regression test for the change, add a
case where the primary loader throws `NoClassDefFoundError` but the context
loader resolves the 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: [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]
