You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2023/02/01 20:53:30 UTC
[GitHub] [kafka] gharris1727 opened a new pull request, #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
gharris1727 opened a new pull request, #13182:
URL: https://github.com/apache/kafka/pull/13182
Plugin path scanning can fail for multiple reasons:
1. A developer incorrectly implements a plugin (e.g. by not providing a default constructor)
2. A packaging mistake happens, and some required class is not present on the plugin.path
3. A plugin throws a runtime exception during initialization or instantiation
In some of these cases, the error will propagate and crash the worker.
In other cases, the error will be caught by the plugin path scanning infrastructure, and the erroneous plugin will not appear in the REST API or be available for running connectors/tasks. This can cause other co-packaged plugins to also be hidden from the REST API.
1. Instead of some errors crashing the worker, all errors from plugin path scanning should be logged, and allow the worker to continue starting up, but with the faulty plugins not available.
2. Instead of errors from one classloader hiding co-packaged plugins, the connect worker should continue scanning for other plugins after a failure.
These should reduce the blast radius of a bad connector deployment down to just that one plugin class not being usable on a cluster, instead of a worker being offline.
This will also help in development scenarios where exceptions and packaging errors are more common, as only tests pertaining the the faulty connector will fail, rather than an entire suite failing because the worker was unable to start.
This PR also adds tests for a variety of failure scenarios, and asserts that they each prevent the scanning from crashing the test, while still failing to load the faulty plugin.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
[GitHub] [kafka] gharris1727 commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1122112042
##########
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:
I changed this and the previous log message so that it wasn't confusing that we were instantiating a SinkConnector.
It is unfortunate that on this branch we don't get to see the `Class<?>` after a failure occurs, and have to rely on the ServiceConfigurationError message to report what specific plugin had an issue.
--
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
[GitHub] [kafka] gharris1727 commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1122110705
##########
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 think that actionable error messages are better than non-actionable ones, especially when they can save the novice user a trip to the internet or risk chasing the wrong problem. They also shouldn't be misleading enough to get in the way of an advanced user.
I do agree that those particular messages were a bit weak though, and those inner messages did not really do a good job of explaining what went wrong. I now know what an ExceptionInInitializerError means, but I do remember being quite confused by it 2-3y ago when I first encountered it.
--
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
[GitHub] [kafka] C0urante commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "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
[GitHub] [kafka] C0urante commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1123292197
##########
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:
I think `BAD_PACKAGING` is probably the best here.
I want to make sure that whoever reads these tests in the future isn't actively misled by anything in them. It's less important that the descriptions/variable names perfectly capture intent, and more important that they don't imply something incorrect.
##########
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:
This is fine, thanks 👍
--
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
[GitHub] [kafka] clolov commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1095729055
##########
connect/runtime/src/test/resources/test-plugins/fail-to-initialize/test/plugins/OuterClass.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package test.plugins;
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.kafka.common.config.ConfigDef;
+import org.apache.kafka.connect.connector.Task;
+import org.apache.kafka.connect.sink.SinkConnector;
+
+/**
+ * Fake plugin class for testing classloading isolation.
+ * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}.
+ * <p>Defines a connector as an non-static inner class, which does not have a default constructor.
Review Comment:
```suggestion
* <p>Defines a connector as a non-static inner class, which does not have a default constructor.
```
```suggestion
* <p>Defines a connector as a non-static inner class, which does not have a default constructor.
```
##########
connect/runtime/src/test/resources/test-plugins/fail-to-initialize/test/plugins/CoLocatedPlugin.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package test.plugins;
+
+import java.util.Map;
+import org.apache.kafka.connect.data.Schema;
+import org.apache.kafka.connect.data.SchemaAndValue;
+import org.apache.kafka.connect.storage.Converter;
+
+/**
+ * Fake plugin class for testing classloading isolation.
+ * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}.
+ * This is a plugin co-located with other poorly packaged plugins, but should be visible despite other errors.
Review Comment:
```suggestion
* <p>This is a plugin co-located with other poorly packaged plugins, but should be visible despite other errors.
```
--
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
[GitHub] [kafka] C0urante commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1123292638
##########
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:
Works for me 👍
--
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
[GitHub] [kafka] gharris1727 commented on a diff in pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13182:
URL: https://github.com/apache/kafka/pull/13182#discussion_r1122107145
##########
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:
This is not a description of the behavior of the plugin, but how it is packaged. It is packaged with the other connectors which fail during plugin discovery. And it is packaged this way because before this patch, throwing from the version method caused other plugins to be shadowed (such as CoLocatedPlugin).
I think that the name of this group of plugins could change though. Are any of `FAIL_DURING_DISCOVERY`, `PACKAGED_WITH_FAILING_PLUGINS`, or `BAD_PACKAGING` better than `FAIL_TO_INITIALIZE`?
Alternatively I could remove the prefix from these constants and keep or change the `fail-to-initialize` directory name.
--
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
[GitHub] [kafka] C0urante merged pull request #13182: KAFKA-14649: Isolate failures during plugin path scanning to single plugin classes
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante merged PR #13182:
URL: https://github.com/apache/kafka/pull/13182
--
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