C0urante commented on code in PR #13971:
URL: https://github.com/apache/kafka/pull/13971#discussion_r1262952038
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java:
##########
@@ -62,7 +62,8 @@
* </li>
* </ul>
* <p>Note: This scanner has a runtime proportional to the number of overall
classes in the passed-in
- * {@link PluginSource} objects, which may be significant for plugins with
large dependencies.
+ * {@link PluginSource} objects, which may be significant for plugins with
large dependencies. For a more performant
+ * implementation, consider using {@link ServiceLoaderScanner} and follow
migration instructions for KIP-898.
Review Comment:
Nit: can link to the KIP from here
```suggestion
* implementation, consider using {@link ServiceLoaderScanner} and follow
migration instructions for
* <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-898%3A+Modernize+Connect+plugin+discovery";>KIP-898</a>.
```
##########
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;
Review Comment:
Nit:
```suggestion
private final PluginScanner scanner;
```
##########
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:
I was wondering about replacing this part with a more rigorous assertion:
```java
Set<String> expectedClasses = new HashSet<>(TestPlugins.pluginClasses());
assertEquals(expectedClasses, classes);
```
But this fails with reflection-based scanning (and succeeds with service
loader-based scanning) because the `VersionMethodThrowsConnector` class gets
picked up by the plugin scanner, but isn't included in the result of
`TestPlugins::pluginClasses`.
If we add a service loader manifest for the `VersionMethodThrowsConnector`
class, then both cases fail. And then, if we change `includeByDefault` for the
`BAD_PACKAGING_VERSION_METHOD_THROWS_CONNECTOR` to `true`, both test cases pass.
Should we add these changes? They give us slightly better coverage and more
helpful assertion messages, and also align the total set of loaded classes when
using each type of scanner (which IIUC matches the ideal scenario that we're
aiming for once users have finished using the [KIP-898 migration
script](https://cwiki.apache.org/confluence/display/KAFKA/KIP-898%3A+Modernize+Connect+plugin+discovery#KIP898:ModernizeConnectplugindiscovery-PluginPathManagementScript)).
--
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
