gharris1727 commented on code in PR #13971:
URL: https://github.com/apache/kafka/pull/13971#discussion_r1265841495


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginScannerTest.java:
##########
@@ -20,79 +20,114 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
 
+@RunWith(Parameterized.class)
 public class PluginScannerTest {
 
+    private enum ScannerType { Reflection, ServiceLoader };
+
     @Rule
     public TemporaryFolder pluginDir = new TemporaryFolder();
 
+    public PluginScanner scanner;
+
+    @Parameterized.Parameters
+    public static Collection<Object[]> parameters() {
+        List<Object[]> values = new ArrayList<>();
+        for (ScannerType type : ScannerType.values()) {
+            values.add(new Object[]{type});
+        }
+        return values;
+    }
+
+    public PluginScannerTest(ScannerType scannerType) {
+        switch (scannerType) {
+            case Reflection:
+                this.scanner = new ReflectionScanner();
+                break;
+            case ServiceLoader:
+                this.scanner = new ServiceLoaderScanner();
+                break;
+            default:
+                throw new IllegalArgumentException("Unknown type " + 
scannerType);
+        }
+    }
+
     @Test
-    public void testLoadingUnloadedPluginClass() {
-        DelegatingClassLoader classLoader = initClassLoader(
+    public void testScanningEmptyPluginPath() {
+        PluginScanResult result = scan(
                 Collections.emptyList()
         );
-        for (String pluginClassName : TestPlugins.pluginClasses()) {
-            assertThrows(ClassNotFoundException.class, () -> 
classLoader.loadClass(pluginClassName));
-        }
+        assertTrue(result.isEmpty());
     }
 
     @Test
-    public void testLoadingPluginClass() throws ClassNotFoundException {
-        DelegatingClassLoader classLoader = initClassLoader(
+    public void testScanningPluginClasses() {
+        PluginScanResult result = scan(
                 TestPlugins.pluginPath()
         );
+        Set<String> classes = new HashSet<>();
+        result.forEach(pluginDesc -> classes.add(pluginDesc.className()));
         for (String pluginClassName : TestPlugins.pluginClasses()) {
-            assertNotNull(classLoader.loadClass(pluginClassName));
-            assertNotNull(classLoader.pluginClassLoader(pluginClassName));
+            assertTrue("Expected " + pluginClassName + "to be discovered but 
it was not",
+                    classes.contains(pluginClassName));

Review Comment:
   > But the current implementation still goes through the 100 tries before 
giving up. I agree with the implementation (better to err on the side of not 
unnecessarily killing the worker), but just wanted to double check--this is 
intentional, right?
   
   Yes it is intentional. This edit should be more clear:
   
   > The heuristic is just there to prevent infinite loops in the 
ServiceLoader-not-making-progress case.  If the exception message happens to 
contain a memory address, or the exception alternates between two different 
messages, we should still **eventually** fail the worker **and not be stuck in 
an infinite loop**.
   
   > Also, I notice that we've added LinkageError to the catch clause around 
invoking Iterator::next. Do we think it's worth adding similar logic to prevent 
infinite loops in the event that a call to Iterator::next that throws an 
exception also fails to advance the iterator? Or is this unreasonable paranoia?
   
   I suppose if we're being paranoid about hasNext, being paranoid about next() 
for JDK8 implementations would also make sense. I've made the error-handling 
function more generic and applied it to all of the ServiceLoader operations 
(including load and iterator, which i've never seen fail)
   



-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to