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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
