[GitHub] [kafka] C0urante commented on a change in pull request #10549: KAFKA-8605 log an error message when we detect multiple copies of sam…
C0urante commented on a change in pull request #10549:
URL: https://github.com/apache/kafka/pull/10549#discussion_r615928396
##
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java
##
@@ -187,17 +192,24 @@ private static PluginClassLoader newPluginClassLoader(
);
}
-private void addPlugins(Collection> plugins, ClassLoader
loader) {
+//visible for testing
+ void addPlugins(Collection> plugins, ClassLoader loader)
{
for (PluginDesc plugin : plugins) {
String pluginClassName = plugin.className();
SortedMap, ClassLoader> inner =
pluginLoaders.get(pluginClassName);
+boolean pluginConflict = false;
if (inner == null) {
inner = new TreeMap<>();
pluginLoaders.put(pluginClassName, inner);
// TODO: once versioning is enabled this line should be moved
outside this if branch
log.info("Added plugin '{}'", pluginClassName);
+} else {
+pluginConflict = true;
}
inner.put(plugin, loader);
+if (pluginConflict) {
+log.error("Detected multiple copies of plugin '{}', one of
these will be used '{}'", pluginClassName, inner.keySet());
+}
Review comment:
Mmmm, I'm not sure we should be making decisions here based on dynamic
plugin loading for two reasons:
1. This change can be backported to older versions of Connect, which will
never have that feature.
2. It's unclear exactly what the mechanism for dynamic plugin loading will
be, and it's possible that a re-scan of all known plugins after loading has
taken place (either the initial start load or a subsequent dynamic load at
runtime) could still be beneficial
Also, it's actually not that uncommon for 3+ copies of the same plugin to
appear on the plugin path for a worker. For example, some connectors come
packaged directly with converters; all it takes is at least two such connectors
and a separately-installed copy of that converter to lead to that number of
copies, without any error or misconfiguration on the part of the user.
--
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
[GitHub] [kafka] C0urante commented on a change in pull request #10549: KAFKA-8605 log an error message when we detect multiple copies of sam…
C0urante commented on a change in pull request #10549:
URL: https://github.com/apache/kafka/pull/10549#discussion_r615928396
##
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java
##
@@ -187,17 +192,24 @@ private static PluginClassLoader newPluginClassLoader(
);
}
-private void addPlugins(Collection> plugins, ClassLoader
loader) {
+//visible for testing
+ void addPlugins(Collection> plugins, ClassLoader loader)
{
for (PluginDesc plugin : plugins) {
String pluginClassName = plugin.className();
SortedMap, ClassLoader> inner =
pluginLoaders.get(pluginClassName);
+boolean pluginConflict = false;
if (inner == null) {
inner = new TreeMap<>();
pluginLoaders.put(pluginClassName, inner);
// TODO: once versioning is enabled this line should be moved
outside this if branch
log.info("Added plugin '{}'", pluginClassName);
+} else {
+pluginConflict = true;
}
inner.put(plugin, loader);
+if (pluginConflict) {
+log.error("Detected multiple copies of plugin '{}', one of
these will be used '{}'", pluginClassName, inner.keySet());
+}
Review comment:
Mmmm, I'm not sure we should be making decisions here based on dynamic
plugin loading for two reasons:
1. This change can be backported to older versions of Connect, which will
never have that feature.
2. It's unclear exactly what the mechanism for dynamic plugin loading will
be, and it's possible that a re-scan of all known plugins after loading has
taken place (either the initial start load or a subsequent dynamic load at
runtime) could still be beneficial
Also, it's actually not that uncommon for 3+ copies of the same plugin to
appear on the plugin path for a worker. For example, some connectors come
packaged directly with converters; all it takes is at least two such connectors
and a separately-installed copy of that converter to lead to that number of
copies.
--
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
[GitHub] [kafka] C0urante commented on a change in pull request #10549: KAFKA-8605 log an error message when we detect multiple copies of sam…
C0urante commented on a change in pull request #10549:
URL: https://github.com/apache/kafka/pull/10549#discussion_r615189609
##
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java
##
@@ -187,17 +192,24 @@ private static PluginClassLoader newPluginClassLoader(
);
}
-private void addPlugins(Collection> plugins, ClassLoader
loader) {
+//visible for testing
+ void addPlugins(Collection> plugins, ClassLoader loader)
{
for (PluginDesc plugin : plugins) {
String pluginClassName = plugin.className();
SortedMap, ClassLoader> inner =
pluginLoaders.get(pluginClassName);
+boolean pluginConflict = false;
if (inner == null) {
inner = new TreeMap<>();
pluginLoaders.put(pluginClassName, inner);
// TODO: once versioning is enabled this line should be moved
outside this if branch
log.info("Added plugin '{}'", pluginClassName);
+} else {
+pluginConflict = true;
}
inner.put(plugin, loader);
+if (pluginConflict) {
+log.error("Detected multiple copies of plugin '{}', one of
these will be used '{}'", pluginClassName, inner.keySet());
+}
Review comment:
Oh, gotcha--in that case, should we do a check somewhere else, since
this will be triggered potentially multiple times for a single plugin? For
example, if there are three copies of a connector, the warning will be logged
twice right now, with different values for `inner.keySet()` each time.
Also, it may also help to log exactly which one we're going to use either
instead of or in addition to the complete set of discovered versions of the
duplicated plugin.
--
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
[GitHub] [kafka] C0urante commented on a change in pull request #10549: KAFKA-8605 log an error message when we detect multiple copies of sam…
C0urante commented on a change in pull request #10549:
URL: https://github.com/apache/kafka/pull/10549#discussion_r615168657
##
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java
##
@@ -187,17 +192,24 @@ private static PluginClassLoader newPluginClassLoader(
);
}
-private void addPlugins(Collection> plugins, ClassLoader
loader) {
+//visible for testing
+ void addPlugins(Collection> plugins, ClassLoader loader)
{
for (PluginDesc plugin : plugins) {
String pluginClassName = plugin.className();
SortedMap, ClassLoader> inner =
pluginLoaders.get(pluginClassName);
+boolean pluginConflict = false;
if (inner == null) {
inner = new TreeMap<>();
pluginLoaders.put(pluginClassName, inner);
// TODO: once versioning is enabled this line should be moved
outside this if branch
log.info("Added plugin '{}'", pluginClassName);
+} else {
+pluginConflict = true;
}
inner.put(plugin, loader);
+if (pluginConflict) {
+log.error("Detected multiple copies of plugin '{}', one of
these will be used '{}'", pluginClassName, inner.keySet());
+}
Review comment:
Do we need the `pluginConflict` field, or can this be moved into the
`else` branch above?
##
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoaderTest.java
##
@@ -129,4 +133,32 @@ public void testLoadingMixOfValidAndInvalidPlugins()
throws Exception {
assertNotNull(classLoader.pluginClassLoader(pluginClassName));
}
}
+
+@Test
+public void testAddMultiplePluginsWithSameClass() {
Review comment:
Is this test buying us much? It looks like it's verifying that multiple
versions of the same plugin are recognized by the worker, but not really
probing how the worker handles that case.
For what it's worth, I think the functional parts of this change are small
and readable enough that a test might not even be warranted.
--
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
