[GitHub] [kafka] gharris1727 commented on a change in pull request #10475: KAFKA-12610: Implement PluginClassLoader::getResource

[GitBox <gi...@apache.org>]

gharris1727 commented on a change in pull request #10475:
URL: https://github.com/apache/kafka/pull/10475#discussion_r607216362



##########
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:
       While I understand that the motivation here for explicitly providing the parent is control the behavior of the parent ClassLoader, I think this test is for a pretty extreme case. This is testing a:
   
   1. PluginClassLoader whose parent is a
   2. DelegatingClassLoader whose parent is a
   3. PluginClassLoader whose parent is a
   4. Delegating ClassLoader whose parent is the
   5. App ClassLoader
   
   Does this test still succeed if we use a different ClassLoader that's not part of a chain like this?
   We could construct a plain `URLClassLoader` instance as the parent, but that would still require the new Plugins constructor.
   If we add a test resource file to the App ClassLoader we could skip the additional constructor. We could use multiple different resource file names in the different test plugins to test the three cases:
   
   1. parent has resource but plugin does not
   2. plugin has resource but parent does not
   3. parent and plugin both have the resource

##########
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()

Review comment:
       nit: TestPlugins has a Map from class name to File, we can add a method for getting the path to a single plugin jar to TestPlugins itself, so we don't have to iterate here.

##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java
##########
@@ -114,6 +118,9 @@
             pluginJars.put(SAMPLING_HEADER_CONVERTER, createPluginJar("sampling-header-converter"));
             pluginJars.put(SAMPLING_CONFIG_PROVIDER, createPluginJar("sampling-config-provider"));
             pluginJars.put(SERVICE_LOADER, createPluginJar("service-loader"));
+            // Create two versions of the same plugin reading version string from a resource
+            pluginJars.put(READ_VERSION_FROM_RESOURCE + ".v1", createPluginJar("read-version-from-resource-v1"));

Review comment:
       nit: make each of these a separate constant




-- 
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:
users@infra.apache.org