[GitHub] [kafka] C0urante commented on a diff in pull request #13771: MINOR: Refactor DelegatingClassLoader to emit immutable PluginScanResult

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

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


##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java:
##########
@@ -356,25 +357,16 @@ public static String prunedName(PluginDesc<?> plugin) {
      * Verify whether a given plugin's alias matches another alias in a collection of plugins.
      *
      * @param alias the plugin descriptor to test for alias matching.
-     * @param plugins the collection of plugins to test against.
+     * @param aliases the collection of plugins to test against.
      * @param <U> the plugin type.
      * @return false if a match was found in the collection, otherwise true.
      */
     public static <U> boolean isAliasUnique(
             PluginDesc<U> alias,
-            Collection<PluginDesc<U>> plugins
+            Map<String, String> aliases
     ) {
-        boolean matched = false;
-        for (PluginDesc<U> plugin : plugins) {
-            if (simpleName(alias).equals(simpleName(plugin))
-                    || prunedName(alias).equals(prunedName(plugin))) {
-                if (matched) {
-                    return false;
-                }
-                matched = true;
-            }
-        }
-        return true;
+        // TODO: Mark alias collision and disable ambiguous aliases completely.

Review Comment:
   > I don't think that anyone should be depending on these cross-plugin ambiguous aliases, because if they were, they would be subject to spurious ClassNotFoundExceptions whenever the non-deterministic iteration order caused a plugin of the wrong type to be loaded.
   
   Plugin scanning and alias computation is currently deterministic (the various `Set<PluginDesc<?>>` objects that we use are all instances of the `TreeSet` type), so users are not subject to this risk while running a fixed version of Kafka Connect with a fixed plugin path (and fixed contents of that plugin path).
   
   That said, I think the changes you've made are sufficient.
   
   > An alternative is to separate these aliases into per-plugin-type namespaces, but I don't think that is necessary at this time.
   
   Yeah, this would be nice to have but it's not a blocker. One case where it could come in handy is with plugins that implement multiple plugin interfaces (which is actually fairly common with `Converter` and `HeaderConverter` instances); you may have multiple `JsonConverter` plugins on your worker, but if only one of them implements the `Converter` interface and another implements the `Converter` and `HeaderConverter` interface, it's hard to argue that you shouldn't be able to use an alias when using the latter as a header converter.



-- 
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