C0urante commented on code in PR #13971: URL: https://github.com/apache/kafka/pull/13971#discussion_r1265781087
########## 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: This all looks great, thanks 👍 One small request for clarification: to me, this implies that we'd fail the worker without trying 100 times to invoke `hasNext`: > If the exception message happens to contain a memory address, or the exception alternates between two different messages, we should still fail the worker. 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? 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? ########## 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: This all looks great, thanks 👍 One small request for clarification: to me, this implies that we'd fail the worker without trying 100 times to invoke `hasNext`: > If the exception message happens to contain a memory address, or the exception alternates between two different messages, we should still fail the worker. 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? 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? -- 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