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