gharris1727 commented on code in PR #13971:
URL: https://github.com/apache/kafka/pull/13971#discussion_r1263122416
##########
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:
Thanks, I added both of your suggested fixes. The missing manifest and
incorrect includeByDefault were oversights on my part, so the stronger test is
a good idea.
Your suggestion also made me realize that I missed some ServiceLoader
entries, meaning that ServiceLoaderScanner tests were incomplete. After I added
the entries, two tests fail with an error (not an assertion) because the
ServiceLoaderScanner doesn't catch the NoClassDefFound (LinkageError) that the
ReflectionScanner does.
I added the failing MissingSuperclassConverter and NonExistentInterface test
case in KAFKA-14649 #13182, to assert that they no longer crashed the worker,
which I thought was reasonable at the time. It appears that the ServiceLoader
takes the opinion that LinkageError (like other Errors) should not be caught
due to the severity of the cause, which is making me second-think catching it
in the ReflectionScanner.
I think there are some options here:
1. Delete the MissingSuperclassConverter manifest to disable the test case
for the ServiceLoaderScanner
2. Catch the LinkageError in ServiceLoaderScanner making it shadow other
plugins in the same PluginLocation.
3. Make ServiceLoaderScanner behave like ReflectionScanner by forking the
ServiceLoader implementation and changing one catch clause.
4. Make ReflectionScanner behave like ServiceLoaderScanner and kill the
worker for missing dependencies for plugin classes, restoring the
pre-KAFKA-14649 behavior for LinkageError.
Of those options, I think i'm going to implement (4), as i'm loathe to fork
ServiceLoader this early in the project, and the severity and breadth of causes
for LinkageError means we probably shouldn't handle it.
What do you think? I've pushed the MissingSuperclassConverter manifest so
the PluginScannerTest is currently failing.
--
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]
