[GitHub] [kafka] C0urante commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes

["C0urante (via GitHub)" <gi...@apache.org>]

C0urante commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1120776026


##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java:
##########
@@ -419,7 +423,14 @@ private <T> Collection<PluginDesc<T>> getServiceLoaderPluginDesc(Class<T> klass,
         Collection<PluginDesc<T>> result = new ArrayList<>();
         try {
             ServiceLoader<T> serviceLoader = ServiceLoader.load(klass, loader);
-            for (T pluginImpl : serviceLoader) {
+            for (Iterator<T> iterator = serviceLoader.iterator(); iterator.hasNext(); ) {
+                T pluginImpl;
+                try {
+                    pluginImpl = iterator.next();
+                } catch (ServiceConfigurationError t) {
+                    log.error("Unable to instantiate plugin{}", reflectiveErrorDescription(t.getCause()), t);

Review Comment:
   Would it be more informative to use the type of plugin class being instantiated instead of just "plugin"?
   ```suggestion
                       log.error("Unable to instantiate {}{}", klass.getSimpleName(), reflectiveErrorDescription(t.getCause()), t);
   ```



##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java:
##########
@@ -111,20 +115,62 @@ public enum TestPlugin {
         /**
          * A plugin which shares a jar file with {@link TestPlugin#MULTIPLE_PLUGINS_IN_JAR_THING_ONE}
          */
-        MULTIPLE_PLUGINS_IN_JAR_THING_TWO("multiple-plugins-in-jar", "test.plugins.ThingTwo");
+        MULTIPLE_PLUGINS_IN_JAR_THING_TWO("multiple-plugins-in-jar", "test.plugins.ThingTwo"),
+        /**
+         * A plugin which is incorrectly packaged, and is missing a superclass definition.
+         */
+        FAIL_TO_INITIALIZE_MISSING_SUPERCLASS("fail-to-initialize", "test.plugins.MissingSuperclass", false, REMOVE_CLASS_FILTER),
+        /**
+         * A plugin which is packaged with other incorrectly packaged plugins, but itself has no issues loading.
+         */
+        FAIL_TO_INITIALIZE_CO_LOCATED("fail-to-initialize", "test.plugins.CoLocatedPlugin", true, REMOVE_CLASS_FILTER),
+        /**
+         * A connector which is incorrectly packaged, and throws during static initialization.
+         */
+        FAIL_TO_INITIALIZE_STATIC_INITIALIZER_THROWS_CONNECTOR("fail-to-initialize", "test.plugins.StaticInitializerThrowsConnector", false, REMOVE_CLASS_FILTER),
+        /**
+         * A plugin which is incorrectly packaged, which throws an exception from the {@link Versioned#version()} method.
+         */
+        FAIL_TO_INITIALIZE_VERSION_METHOD_THROWS_CONNECTOR("fail-to-initialize", "test.plugins.VersionMethodThrowsConnector", false, REMOVE_CLASS_FILTER),

Review Comment:
   Nit: d'you think `FAIL_TO_INITIALIZE` is a bit inaccurate here, since we do manage to load and use the plugin despite its version method throwing an exception?



##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java:
##########
@@ -439,6 +457,22 @@ public static <T> String versionFor(Class<? extends T> pluginKlass) throws Refle
             versionFor(pluginKlass.getDeclaredConstructor().newInstance()) : UNDEFINED_VERSION;
     }
 
+    private static String reflectiveErrorDescription(Throwable t) {
+        if (t instanceof NoSuchMethodException) {
+            return ": Plugin class must have a default constructor, and cannot be a non-static inner class";
+        } else if (t instanceof SecurityException) {
+            return ": Security settings must allow reflective instantiation of plugin classes";
+        } else if (t instanceof IllegalAccessException) {
+            return ": Plugin class default constructor must be public";
+        } else if (t instanceof ExceptionInInitializerError) {
+            return ": Plugin class should not throw exception during static initialization";
+        } else if (t instanceof InvocationTargetException) {
+            return ": Constructor must complete without throwing an exception";
+        } else {
+            return "";

Review Comment:
   I like this method for the most part; it helps provide a nice summary of what's wrong with the plugin class and how to fix things.
   
   One thought I had is that it may be better in some if not all cases to describe the problem rather than prescribe a solution. For example, "Constructor must complete without throwing an exception" isn't really groundbreaking information; it's probably fine to just say "Failed to invoke constructor" and let users figure out the rest from the remainder of the stack trace.
   
   I think this applies to the branches for `ExceptionInInitializerError` and `InvocationTargetException` classes, and the others could be left as-are, but feel free to tweak the wording if they could be improved as well.



-- 
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-unsubscribe@kafka.apache.org

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