ncliang commented on a change in pull request #10475:
URL: https://github.com/apache/kafka/pull/10475#discussion_r607232615
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java
##########
@@ -342,6 +343,47 @@ public void
newPluginsShouldConfigureWithPluginClassLoader() {
assertPluginClassLoaderAlwaysActive(samples);
}
+ @Test
+ public void pluginClassLoaderReadVersionFromResource() {
+ TestPlugins.assertAvailable();
+
+ Map<String, String> pluginProps = new HashMap<>();
+ pluginProps.put(WorkerConfig.PLUGIN_PATH_CONFIG,
+ TestPlugins.pluginPath().stream()
+ .filter(s ->
s.contains("read-version-from-resource-v1"))
+ .collect(Collectors.joining()));
+ plugins = new Plugins(pluginProps);
+
+ Converter converter = plugins.newPlugin(
+ TestPlugins.READ_VERSION_FROM_RESOURCE,
+ new AbstractConfig(new ConfigDef(), Collections.emptyMap()),
+ Converter.class
+ );
+ assertEquals("1.0.0",
+ new String(converter.fromConnectData(null, null, null)));
+ PluginClassLoader pluginClassLoader = plugins.delegatingLoader()
+ .pluginClassLoader(TestPlugins.READ_VERSION_FROM_RESOURCE);
+ assertNotNull(pluginClassLoader);
+
+
+ // Re-initialize Plugins object with plugin class loader in the class
loader tree. This is
+ // to simulate the situation where jars exist on both system classpath
and plugin path.
+ pluginProps.put(WorkerConfig.PLUGIN_PATH_CONFIG,
+ TestPlugins.pluginPath().stream()
+ .filter(s ->
s.contains("read-version-from-resource-v2"))
+ .collect(Collectors.joining()));
+ plugins = new Plugins(pluginProps, pluginClassLoader);
Review comment:
I agree that having the additional testcases would be valuable. However,
I do think that adding the resource directly to the app classloader makes the
test much less readable. I like the idea of constructing and using a plain
`URLClassLoader` as the parent. While it still requires the additional
constructor to be exposed on `Plugins`, we do already expose the parent
parameter on `DelegatingClassLoader` for specifically the flexibility of
controlling the parent loader.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
