gharris1727 commented on a change in pull request #10475:
URL: https://github.com/apache/kafka/pull/10475#discussion_r621586391
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java
##########
@@ -98,22 +97,56 @@
* to load internal classes, and samples information about their
initialization.
*/
public static final String SERVICE_LOADER =
"test.plugins.ServiceLoaderPlugin";
+ /**
+ * Class name of a plugin which reads a version string from resource.
+ */
+ public static final String READ_VERSION_FROM_RESOURCE =
"test.plugins.ReadVersionFromResource";
private static final Logger log =
LoggerFactory.getLogger(TestPlugins.class);
- private static final Map<String, File> PLUGIN_JARS;
+ private static final Map<String, PluginJar> PLUGIN_JARS;
private static final Throwable INITIALIZATION_EXCEPTION;
+ private static final class PluginJar {
+ String className;
+ File jarFile;
+
+ public PluginJar(String className, File jarFile) {
+ this.className = className;
+ this.jarFile = jarFile;
+ }
+
+ public String getClassName() {
+ return className;
+ }
+
+ public File getJarFile() {
+ return jarFile;
+ }
+ }
+
static {
Throwable err = null;
- HashMap<String, File> pluginJars = new HashMap<>();
+ Map<String, PluginJar> pluginJars = new HashMap<>();
try {
- pluginJars.put(ALWAYS_THROW_EXCEPTION,
createPluginJar("always-throw-exception"));
- pluginJars.put(ALIASED_STATIC_FIELD,
createPluginJar("aliased-static-field"));
- pluginJars.put(SAMPLING_CONVERTER,
createPluginJar("sampling-converter"));
- pluginJars.put(SAMPLING_CONFIGURABLE,
createPluginJar("sampling-configurable"));
- 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"));
+ pluginJars.put("always-throw-exception",
+ new PluginJar(ALWAYS_THROW_EXCEPTION,
createPluginJar("always-throw-exception")));
+ pluginJars.put("aliased-static-field",
+ new PluginJar(ALIASED_STATIC_FIELD,
createPluginJar("aliased-static-field")));
+ pluginJars.put("sampling-converter",
+ new PluginJar(SAMPLING_CONVERTER,
createPluginJar("sampling-converter")));
+ pluginJars.put("sampling-configurable",
+ new PluginJar(SAMPLING_CONFIGURABLE,
createPluginJar("sampling-configurable")));
+ pluginJars.put("sampling-header-converter",
+ new PluginJar(SAMPLING_HEADER_CONVERTER,
createPluginJar("sampling-header-converter")));
+ pluginJars.put("sampling-config-provider",
+ new PluginJar(SAMPLING_CONFIG_PROVIDER,
createPluginJar("sampling-config-provider")));
+ pluginJars.put("service-loader",
+ new PluginJar(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",
Review comment:
Since these string literals are now relevant elsewhere, we should make
them reusable constants. Perhaps they should be enums? I realize now that
perhaps the class names should have also been enums but 🤷.
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java
##########
@@ -143,16 +176,35 @@ public static void assertAvailable() throws
AssertionError {
public static List<String> pluginPath() {
Review comment:
The existing structure of this class bakes in the assumption that each
plugin only appears once on the plugin path, and that the common use-case
plugin path (returned by this method) is then valid. This change would make
this method return an invalid plugin path, with flakey behavior when loading
the duplicated plugin (which plugin gets loaded would be determined by
iteration order over the PLUGIN_JARS hashmap).
There are a couple of ways out:
1. avoid tackling this now, and separate the two plugins with different
class names
2. have this method pick one of the duplicates to drop deterministically, so
that the Plugins class doesn't have undefined loading behavior.
3. allow/deny some of these plugins from being included in this default
plugin path, and keep some plugins back for the more specific tests
4. remove this method, and have PluginsTest explicitly include the needed
plugins in each test, and/or a default list to include if none are specifically
requested.
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java
##########
@@ -98,22 +97,56 @@
* to load internal classes, and samples information about their
initialization.
*/
public static final String SERVICE_LOADER =
"test.plugins.ServiceLoaderPlugin";
+ /**
+ * Class name of a plugin which reads a version string from resource.
+ */
+ public static final String READ_VERSION_FROM_RESOURCE =
"test.plugins.ReadVersionFromResource";
private static final Logger log =
LoggerFactory.getLogger(TestPlugins.class);
- private static final Map<String, File> PLUGIN_JARS;
+ private static final Map<String, PluginJar> PLUGIN_JARS;
private static final Throwable INITIALIZATION_EXCEPTION;
+ private static final class PluginJar {
+ String className;
+ File jarFile;
+
+ public PluginJar(String className, File jarFile) {
Review comment:
Perhaps another thing that should be representable by the data structure
in this class is a jar file which contains multiple plugins. This is a
completely supported use-case, but we don't have any tests related to it. If
we're adding the ability to represent an unsupported configuration, maybe we
should make sure it's able to represent other supported use-cases too :)
--
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:
us...@infra.apache.org
