gharris1727 commented on code in PR #18403:
URL: https://github.com/apache/kafka/pull/18403#discussion_r1905857476
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginScannerTest.java:
##########
@@ -137,6 +139,17 @@ public void testVersionedPluginsHasVersion(PluginScanner
scanner) {
versionedPluginResult.forEach(pluginDesc -> assertEquals("1.0.0",
pluginDesc.version()));
}
+ @ParameterizedTest
+ @MethodSource("parameters")
+ public void testClasspathPluginIsAlsoLoadedInIsolation(PluginScanner
scanner) {
+ // json converter is part of the classpath by default
+ String jsonConverterLocation =
JsonConverter.class.getProtectionDomain().getCodeSource().getLocation().getPath();
Review Comment:
This feels very brittle to me.
Rather than using a real class, can this use a TestPlugin with a similar
name to a classpath plugin?
Or the last time we needed to set up a "classpath" plugin, it was by
injecting a new loader in the hierarchy in
`PluginsTest#assertClassLoaderReadsVersionFromResource`.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java:
##########
@@ -76,8 +78,21 @@ private static <T> String versionFor(Class<? extends T>
pluginKlass) throws Refl
@Override
protected PluginScanResult scanPlugins(PluginSource source) {
+ // By default, java and classgraph uses parent first classloading,
hence if a plugin is loaded by the classpath
+ // loader, and then by an isolated plugin loader, the default
precedence will always load the classpath version.
+ // This breaks isolation and hence connect uses isolated plugin
loaders, which are child first classloaders.
+ // Therefore, we override the classloader order to be child first, so
that the isolated plugin loader is used first.
+ // In addition, we need to explicitly specify the full classloader
order, as classgraph only scans the classes available
+ // in the classloaders and not the entire parent chain. Due to this
reason if a plugin is extending a class present
+ // in classpath/application it will not be able to find the parent
class unless we explicitly specify the classloader order.
+ List<ClassLoader> classLoaderOrder = new ArrayList<>();
+ ClassLoader cl = source.loader();
+ while (cl != null) {
+ classLoaderOrder.add(cl);
+ cl = cl.getParent();
+ }
Review Comment:
Is this true? What is the `ignoreParentClassLoaders` method doing then?
--
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]
