KAFKA-15030: Add connect-plugin-path command-line tool.

[gharris1727]

This adds only the list subcommand, the sync-manifests subcommand will be in a follow-up PR.

This includes new dependencies to the tools package on connect-runtime and connect-runtime-test in order to test the functionality of this command. New entry points for linux (.sh) and windows (bat) are added as well.

This script runs the Reflective and ServiceLoader scans used at worker startup, as well as directly reading the manifest files. This is in order to find plugins which have manifests but are not loadable. The alternative of having the PluginScanner emit erroneous plugins would not include the locations of the manifest files without a custom ServiceLoader implementation. The locations of the manifest files are necessary later for the sync-manifests command, which will directly re-write these manifests.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[gharris1727]

Here's some sample output from the current implementation:

$ ./bin/connect-plugin-path.sh list --plugin-path ~/test/plugin-path/
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/Users/greg.harris/github/kafka/tools/build/dependant-libs-2.13.11/slf4j-reload4j-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/Users/greg.harris/github/kafka/trogdor/build/dependant-libs-2.13.11/slf4j-reload4j-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Reload4jLoggerFactory]
/Users/greg.harris/test/plugin-path/a2solutions-oracdc-kafka-1.3.3.1    solutions.a2.cdc.oracle.OraCdcJdbcSinkConnector OraCdcJdbcSink  OraCdcJdbcSinkConnector 1.3.3.1 sink    true    false
/Users/greg.harris/test/plugin-path/a2solutions-oracdc-kafka-1.3.3.1    solutions.a2.cdc.oracle.OraCdcLogMinerConnector OraCdcLogMinerConnector OraCdcLogMiner  1.3.3.1 source  true    false
/Users/greg.harris/test/plugin-path/a2solutions-oracdc-kafka-1.3.3.1    solutions.a2.cdc.oracle.OraCdcSourceConnector   OraCdcSource    OraCdcSourceConnector   1.3.3.1 source  true    false
/Users/greg.harris/test/plugin-path/microsoft-kafka-connect-iothub-0.6  com.microsoft.azure.iot.kafka.connect.sink.IotHubSinkConnector  IotHubSink      IotHubSinkConnector     0.6     sink    true    false
/Users/greg.harris/test/plugin-path/microsoft-kafka-connect-iothub-0.6  com.microsoft.azure.iot.kafka.connect.IotHubSourceConnector     IotHubSourceConnector   IotHubSource    0.6     source  true    false
[2023-07-20 14:19:13,855] ERROR Failed to get plugin version for class com.github.jcustenborder.kafka.connect.spooldir.elf.SpoolDirELFSourceConnector (org.apache.kafka.connect.runtime.isolation.PluginScanner)
java.lang.NoClassDefFoundError: com/google/common/base/Strings
        at com.github.jcustenborder.kafka.connect.utils.VersionUtil.version(VersionUtil.java:32)
        at com.github.jcustenborder.kafka.connect.spooldir.elf.SpoolDirELFSourceConnector.version(SpoolDirELFSourceConnector.java:57)
        at org.apache.kafka.connect.runtime.isolation.PluginScanner.versionFor(PluginScanner.java:199)
        at org.apache.kafka.connect.runtime.isolation.ReflectionScanner.versionFor(ReflectionScanner.java:74)
        at org.apache.kafka.connect.runtime.isolation.ReflectionScanner.getPluginDesc(ReflectionScanner.java:135)
        at org.apache.kafka.connect.runtime.isolation.ReflectionScanner.scanPlugins(ReflectionScanner.java:88)
        at org.apache.kafka.connect.runtime.isolation.PluginScanner.scanUrlsAndAddPlugins(PluginScanner.java:79)
        at org.apache.kafka.connect.runtime.isolation.PluginScanner.discoverPlugins(PluginScanner.java:67)
        at org.apache.kafka.tools.ConnectPluginPath.runCommand(ConnectPluginPath.java:213)
        at org.apache.kafka.tools.ConnectPluginPath.mainNoExit(ConnectPluginPath.java:79)
        at org.apache.kafka.tools.ConnectPluginPath.main(ConnectPluginPath.java:71)
Caused by: java.lang.ClassNotFoundException: com.google.common.base.Strings
        at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:587)
        at org.apache.kafka.connect.runtime.isolation.PluginClassLoader.loadClass(PluginClassLoader.java:136)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
        ... 11 more

The SLF4J lines and the ERROR logs are printed to stderr, so redirecting with 2> /dev/null is sufficient to clean up the output. Unloadable plugins (due to class structure problems) are printed with error logs.

Right now the stdout is formatted like a TSV, but without a header. The KIP only specifies that this output be human-readable, so i'm interested in changing this output slightly before merging. The columns are:

• plugin location
• full class name
• simple name (if unique within the classpath+location), or null
• pruned name (if unique within the classpath+location), or null
• reported version, or undefined
• plugin type (sink/source/converter/etc)
• whether the plugin is reflectively loadable
• whether the plugin has a manifest file

Because the ReflectionScanner is used on individual locations one-at-a-time, the script is slow, but prints results after scanning each location. For the above, the results start to be printed within about 1/4 second, and the whole listing takes 30s for 150 components (400 plugins) downloaded from the internet.

[C0urante]

Thanks Greg! Left some comments, mostly small stylistic stuff.

Higher-level thoughts here:

I think TSV is fine for the output format. It'd definitely help if we added headers. I also found it a bit difficult to parse with the plugin location being listed first; I know that it makes sense in a hierarchical sense (plugin locations contain one or more plugin classes, each of which implements one or more plugin types), but it felt a little impractical since it's arguably the least important piece of information. I think it'd make more sense to place the plugin location at the end of the output, or at least after the plugin's fully-qualified class name, type, and whether it's loadable and has a manifest.

When run on a worker config with no plugin.path property, there's no output. We should at least log a warning in this case, and if there's no legitimate reason to operate on a worker config without a plugin.path property, we should probably fail instead.

More generally, if no plugins are detected, the script exits with no output. Perhaps we could summarize how many plugins were found at the end of the script?

When run on a worker config with plugin.path set to a blank string (i.e., plugin.path=), then the script scans the current directory. Since this is also the behavior for Connect workers, I don't think we can disallow this case, but maybe it's worth printing a warning (in both cases)?

It seems like we're introducing divergence here between the plugin path CLI and Connect workers in how aliases are computed. Workers compute aliases after doing complete scanning of the plugin path, whereas this script does it for each plugin location. Doesn't that introduce false negatives into the script's collision detection logic? FWIW, I think it's fine if we report aliases even if there are collisions (I know that the KIP uses the language "Plugin aliases (if available)" to describe the output of the list command, but I assumed this mean "if a pruned name could be constructed", not "if no collisions were detected").

Nearly all of the out-of-the-box plugins for Connect (SMTs, client config override policies, predicates, converters, header converters) have undefined versions. Can we implement the Versioned interface in these plugins so that they show up with valid version info? (If so, it's fine to leave that for a follow-up PR).

If the CLI picks up the JSON converter, it prints the same information for that plugin twice:

path/to/kafka/connect/json/build/libs/connect-json-3.6.0-SNAPSHOT.jar org.apache.kafka.connect.json.JsonConverter JsonConverter Json undefined converter true false
path/to/apache/kafka/connect/json/build/libs/connect-json-3.6.0-SNAPSHOT.jar org.apache.kafka.connect.json.JsonConverter JsonConverter Json undefined converter true false

IIUC this is because the JSON converter implements two different plugin interfaces (Converter and HeaderConverter), but the PluginDesc class is only capable of storing one plugin type for each kind of plugin. It hasn't been (much of) a problem til now, but we should probably address that somehow before merging this PR.

Finally, I could be missing something, but it seems like we don't have logic to ensure that plugins which implement multiple plugin interfaces (like the aforementioned JSON converter) have all of the necessary service loader manifests; instead, it looks like we only check for at least one manifest. If that's the case, can we start reporting these plugins as not having manifests, in the output lines for the plugin types that they're missing manifests for?

[gharris1727]

I think TSV is fine for the output format. It'd definitely help if we added headers.

👍

I also found it a bit difficult to parse with the plugin location being listed first

That has been bothering me too, so i'm glad you mentioned it. Especially when the plugins are deep in the filesystem tree, the constant part of the path takes up the whole screen. I'll move it.

When run on a worker config with no plugin.path property, there's no output. We should at least log a warning in this case, and if there's no legitimate reason to operate on a worker config without a plugin.path property, we should probably fail instead.

Thanks for testing this. I'm conflicted, because we disallow omitting all of the the location, path, and config arguments at the same time in an attempt to avoid empty listings. Since an empty plugin.path is permitted in a valid worker config without a warning, I think we also have to permit it here without a warning. We'll trust the user says what they mean, and if they don't actually mean what they say, they'll learn about that with an empty result.

More generally, if no plugins are detected, the script exits with no output. Perhaps we could summarize how many plugins were found at the end of the script?

I have been doing that with | wc -l, which becomes a bit messy with the addition of the header. Perhaps following the direct listing with a separator that is easy to parse (an empty line), and then printing a summary (# of plugins, # of classes, # of sink connectors, # that will be changed by sync-manifests, etc) would be appealing. That way, anyone that wants to process the raw data themselves can cut off the summary at the separator. I'll explore this a bit more.

When run on a worker config with plugin.path set to a blank string (i.e., plugin.path=), then the script scans the current directory. Since this is also the behavior for Connect workers, I don't think we can disallow this case, but maybe it's worth printing a warning (in both cases)?

Oh no, I didn't realize that we accepted relative paths in the plugin.path, but it makes sense given the re-use of the PluginUtils#pluginLocations. Since this command will very likely be called from a different working directory than the worker itself, I think adding a warning for this is a good idea.

It seems like we're introducing divergence here between the plugin path CLI and Connect workers in how aliases are computed. Workers compute aliases after doing complete scanning of the plugin path, whereas this script does it for each plugin location. Doesn't that introduce false negatives into the script's collision detection logic?

I justified it to myself that it was the "most generous" set of aliases, counting only collisions internal to each individual plugin location. But that isn't even correct with the current implementation, since collisions with the classpath also cause aliases to be hidden! I think consistency with the runtime is more important, so I'll change this to compute aliases over the whole run. It's so sad to lose the incremental output though, since the aliases of the first line printed depend on the classname used in the final plugin found.

Nearly all of the out-of-the-box plugins for Connect (SMTs, client config override policies, predicates, converters, header converters) have undefined versions. Can we implement the Versioned interface in these plugins so that they show up with valid version info? (If so, it's fine to leave that for a follow-up PR).

This is an amazing suggestion, and I'll certainly implement that in a follow-up PR.

IIUC this is because the JSON converter implements two different plugin interfaces (Converter and HeaderConverter), but the PluginDesc class is only capable of storing one plugin type for each kind of plugin. It hasn't been (much of) a problem til now, but we should probably address that somehow before merging this PR.

Haha yeah that needs some tweaking. I'm worried about the other effects of changing that line, i'll have to look into that more.

Finally, I could be missing something, but it seems like we don't have logic to ensure that plugins which implement multiple plugin interfaces (like the aforementioned JSON converter) have all of the necessary service loader manifests; instead, it looks like we only check for at least one manifest. If that's the case, can we start reporting these plugins as not having manifests, in the output lines for the plugin types that they're missing manifests for?

Well, if the PluginDesc constructor was consistent with equals, then if you had a plugin implement multiple interfaces it would show up in two PluginDesc objects, one for each type. Then, as the script iterates over the PluginScanResult, it would perform the migration once as one type and then again as the other type. I think this is broken because of the PluginDesc constructor, so i'll adjust that and then the migration should work correctly.

@C0urante Thank you for being so thorough, these are details that are really important to get right. That goes for all of the other PRs in this KIP, the feature wouldn't be half as reliable without your scrutiny. I'll take some time to iterate on this and let you know when this is ready for another pass.

[gharris1727]

#14089 contains the fix for the PluginType inference which was pointed out above, it is not necessary for this change but is for the follow-up.

[C0urante]

Hmmm... getting an NPE when I try to run the command now:

Cannot invoke "java.util.Set.removeIf(java.util.function.Predicate)" because the return value of "java.util.Map.get(Object)" is null
java.lang.NullPointerException: Cannot invoke "java.util.Set.removeIf(java.util.function.Predicate)" because the return value of "java.util.Map.get(Object)" is null
	at org.apache.kafka.tools.ConnectPluginPath.lambda$enumerateRows$5(ConnectPluginPath.java:289)
	at java.base/java.lang.Iterable.forEach(Iterable.java:75)
	at org.apache.kafka.connect.runtime.isolation.PluginScanResult.lambda$forEach$0(PluginScanResult.java:136)
	at java.base/java.util.Arrays$ArrayList.forEach(Arrays.java:4204)
	at org.apache.kafka.connect.runtime.isolation.PluginScanResult.forEach(PluginScanResult.java:136)
	at org.apache.kafka.tools.ConnectPluginPath.enumerateRows(ConnectPluginPath.java:285)
	at org.apache.kafka.tools.ConnectPluginPath.lambda$runCommand$2(ConnectPluginPath.java:234)
	at java.base/java.util.stream.Collectors.lambda$uniqKeysMapAccumulator$1(Collectors.java:180)
	at java.base/java.util.stream.ReduceOps$3ReducingSink.accept(ReduceOps.java:169)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1845)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at org.apache.kafka.tools.ConnectPluginPath.runCommand(ConnectPluginPath.java:233)
	at org.apache.kafka.tools.ConnectPluginPath.mainNoExit(ConnectPluginPath.java:84)
	at org.apache.kafka.tools.ConnectPluginPath.main(ConnectPluginPath.java:76)

This happens with both the --plugin-location and --plugin-path flags, when performed on directories that worked well with the CLI on previous commits.

Regarding "following the direct listing with a separator that is easy to parse (an empty line), and then printing a summary (# of plugins, # of classes, # of sink connectors, # that will be changed by sync-manifests, etc)" -- love this, great idea 👍

Regarding the discussion on aliases and eager printing--what if we printed the full set of aliases for each plugin without doing any collision detection, which would allow us to print things eagerly, but then made a note of any collisions that we detected in the summary section? IMO it's more valuable to print things quickly than to have completely-accurate, collision-free info on aliases, especially if running the script against a large plugin path.

As far as everything else goes--unlike most reviews, I'm doing a lot of manual testing for this one, so I'll hold off on another complete round until we can resolve the NPE issue; LMK if you're not getting the same thing and I can try to figure out what's going wrong on my end.

[gharris1727]

Hmmm... getting an NPE when I try to run the command now:

I've fixed that, sorry. I've also added tests that would catch that NPE.

made a note of any collisions that we detected in the summary section

This is a better solution than hiding it in the row listing, as the collisions are much more apparent. I've ported the DEBUG log contents to stdout, and made it part of the summary.

See the new summary below:

Ignoring ambiguous alias 'ByteArray' since it refers to multiple distinct plugins [org.apache.kafka.connect.converters.ByteArrayConverter, io.debezium.converters.ByteArrayConverter, io.confluent.connect.cloud.storage.source.format.ByteArrayConverter, io.confluent.connect.replicator.util.ByteArrayConverter]
<snipped>
Ignoring ambiguous alias 'ByteArrayConverter' since it refers to multiple distinct plugins [org.apache.kafka.connect.converters.ByteArrayConverter, io.debezium.converters.ByteArrayConverter, io.confluent.connect.cloud.storage.source.format.ByteArrayConverter, io.confluent.connect.replicator.util.ByteArrayConverter]
Total plugins:                  419
Loadable plugins:               419
Compatible plugins:             7
Total locations:                150
Compatible locations:           10
All locations compatible?       false

I had to duplicate some of the alias computation logic, since the PluginUtils implementation is tightly coupled to the PluginDesc objects, which we can't use in the summary. If we keep the PluginScanResults from the scanning portion, we end up leaking enough memory that the default 256MB heap runs out of space. Now the script is incremental (again) and can iterate over multi-gb plugin paths with a bounded heap, less than the corresponding worker at least (which gets a 2GB heap by default).

Unfortunately, if there are JDBC drivers included in a plugin, then it will leak memory until the script exits. I don't expect this will be an issue in practice (i'm already testing plugins including JDBC drivers), and people can always increase their heap to compensate.

[C0urante]

Thanks Greg! I'm glad that we've managed to preserve eager printing (if nothing else, it's made this much easier to test locally 😄).

I gave this another shot and it seems like the logic for detecting compatible plugins isn't working. I tried running bin/connect-plugin-path.sh list --plugin-location connect/basic-auth-extension/build/libs/ after building the basic auth extension and got this output (omitting the SLF4J warning messages emitted to stderr):

pluginName	firstAlias	secondAlias	pluginVersion	pluginType	isLoadable	hasManifest	pluginLocation
org.apache.kafka.connect.rest.basic.auth.extension.BasicAuthSecurityRestExtension	BasicAuthSecurityRestExtension	null	3.6.0-SNAPSHOT	rest_extension	true	false	connect/basic-auth-extension/build/libs

Total plugins:           	1
Loadable plugins:        	1
Compatible plugins:      	0
Total locations:         	1
Compatible locations:    	0
All locations compatible?	false

IIUC this plugin should be compatible as we already package a manifest with it here.

As far as the epilogue output goes, I wonder if we're getting a little too detailed here... will users get much out of knowing the number of total locations/compatible locations?

I think the duplication for the alias computation logic is okay. We could throw generics/Java 8-style functional programming/new interfaces at the problem if we really wanted to put all that logic in one place, but I agree that it's not worth the bother right now.

Thanks for noting the JDBC driver memory leak. I agree that this should be fine for now 👍

[gharris1727]

I gave this another shot and it seems like the logic for detecting compatible plugins isn't working. I tried running bin/connect-plugin-path.sh list --plugin-location connect/basic-auth-extension/build/libs/ after building the basic auth extension and got this output (omitting the SLF4J warning messages emitted to stderr):

This causes the PluginClassLoader#getResources() to return the same URL twice, because the same jar path is visible via the plugin location and the classpath. I use the equality of two manifests URLs to exclude ManifestEntry objects that come from the classpath, causing the ManifestEntry for the basic auth extension in the plugin location to be excluded erroneously.

I tested this and it worked:

cp -r connect/basic-auth-extension/build/libs basic-auth-extension-copy
bin/connect-plugin-path.sh list --plugin-location basic-auth-extension-copy

I'll have to figure out a way to distinguish these URLs when the same exact jar is in the classpath and plugin locations.

[gharris1727]

As far as the epilogue output goes, I wonder if we're getting a little too detailed here... will users get much out of knowing the number of total locations/compatible locations?

My justification for it was that a pluginLocation corresponds to an installed artifact for the typical users, and is a proxy for how many dependencies they have. I think this is useful context for the user of this tool to have, and it can also encourage healthy migration practices.

For example, an SMT multi-tool package with tens of individual plugins may all be addressed at once with an upgrade. It will inflate the number of non-compatible plugins considerably, but only add 1 to the non-compatible locations. If someone uses list and sees that tens or hundreds of plugins are incompatible, they may skip the upgrading/contacting steps of the migration and just run sync-manifests. If they see that two or three locations are incompatible, they may be more inclined to upgrade first, knowing the number of dependencies they need to audit is small.

[gharris1727]

Okay I have a potential fix: we can make the URL unique by inserting . in the path.

It works with the test case you provided, and even this one:

 cd connect/basic-auth-extension/build/libs
 ../../../../bin/connect-plugin-path.sh list --plugin-location .
 cd ../../../..

[C0urante]

Hmm... seems like we're still failing to detect missing manifests for plugins that implement multiple interfaces. I added a Converter manifest for the JSON converter (but not a HeaderConverter manifest), then ran the tool:

> bin/connect-plugin-path.sh list --plugin-location connect/json/build/libs/
pluginName	firstAlias	secondAlias	pluginVersion	pluginType	isLoadable	hasManifest	pluginLocation
org.apache.kafka.connect.json.JsonConverter	JsonConverter	Json	undefined	converter	true	true	connect/json/build/libs
org.apache.kafka.connect.json.JsonConverter	JsonConverter	N/A	undefined	header_converter	true	true	connect/json/build/libs

Total plugins:           	2
Loadable plugins:        	2
Compatible plugins:      	2
Total locations:         	1
Compatible locations:    	1
All locations compatible?	true

I also tried moving the JSON converter JAR to a different directory so that it'd be off of the worker classpath, and received the same result.

[C0urante]

As far as listing compatible plugin locations goes, thanks for providing your rationale. I'm convinced that if we can accurately report this, it'd be worth it to include that information 👍

However, I'm worried if we're reporting false (or at least, misleading) positives in cases where all plugins in a given location both cannot be loaded and do not have a manifest. If that occurs, do we report that location as compatible? And if so, isn't that likely to confuse users?

[gharris1727]

I think our workaround with a special path of classpath to denote classpath plugins in PluginUtils::classpathPluginSource should be improved. I built the basic auth extension, created a directory named classpath, moved the JAR file for the extension into that directory, and then ran bin/connect-plugin-path.sh list --plugin-location classpath. The end result was fairly confusing:

Thanks for finding that, I've changed it so that PluginSource uses null as a sentinel value for the classpath, and changed all of the places it's printed to re-inject the "classpath" string after all the control flow has used null.

I've replaced the makeUnique hack with Lists and counting instead of trying to make the ManifestEntry objects completely unique.

I've also finally gotten around to looking at the tests. It seems like we have a great foundation of scenarios to build on, but the assertions we're making are fairly minor. Can we add some coverage to verify, e.g., how many plugin rows are listed, the (partial or complete) contents of the post-table summary, and which plugins are described as being migrated vs. non-migrated?

Yes the assertions are pretty sparse. I've been avoiding parsing the output while it was still in flux, but I think the format is starting to settle down now. I'll go through and see what I can add.

[gharris1727]

I've also added a new warning to stdout which alerts users when they have incompatible plugins on the classpath that will not be auto-migrated. Currently without any manifests in the runtime (they're in a different PR) this is what it looks like:

46 plugins on the classpath are not compatible, and will not be auto-migrated. Please move these plugins to a plugin path location.
        org.apache.kafka.connect.transforms.ReplaceField$Value
        org.apache.kafka.connect.converters.FloatConverter
<snipped>
        org.apache.kafka.connect.converters.ShortConverter
        org.apache.kafka.connect.transforms.predicates.TopicNameMatches
Total plugins:                  1
Loadable plugins:               1
Compatible plugins:             1
Total locations:                1
Compatible locations:           1
All locations compatible?       true

They aren't part of the main table, and they aren't part of the summary.

[gharris1727]

Looking forward at the sync-manifests implementation, I realized that I had some duplication. The ManifestEntry, findManifests function, and the exclude behavior I added in this PR are all redundant and will be replaced with a different system in the next PR.

The new implementation has a lot more machinery included with it that would be un-motivated in this PR, so I'm going to leave this current but obsolete implementation in place for now.

[C0urante]

Thanks Greg, we're really close! Love the new testing coverage.

Sorry for neglecting the earlier discussion about "Compatible locations"; missed the follow-up in my last review. I understand the intent behind this info, but I don't think it's valid to assume that all of our users will.

Regarding this comment:

A plugin which is not loadable and does not have a manifest will not appear in the listing, and is vacuously considered compatible. This is because the output of the PluginScanners cannot distinguish between a class which does not exist and which does exist but has some error during loading.

I don't think vacuously compatible is a useful concept to users, and I especially don't think that justifying any of our behavior with internal implementation details is going to do anybody any favors.

We may choose to report broken plugin locations (i.e., any that has one or more unloadable plugins), but IMO it's better to leave this info out entirely. We have room to maneuver in the future with this tool if we want (the public interface boundaries set by the KIP are very permissive), and I'd rather we err on the side of simplicity for now. Total/loadable/compatible plugins seems like plenty.

I'm also worried about our alias collision reporting. Right now every invocation of the command so far has produced a ton of warnings about SMT aliases collisiding. It'd be nice if we could use ReplaceField$Key as an alias instead of just Key, but until/unless that KIP gets approved, we're stuck with a fairly large number of collisions with the aliases for our OOTB transforms.

These warnings remind me of the "unused configuration" warnings that, until fairly recently, were the ire of Connect users everywhere.

IMO it's not necessary to include information on alias conflicts and we could choose to leave this out entirely. However, if you feel differently and would like to preserve this information, I think it's a hard blocker that we not emit inactionable warnings under routine circumstances (i.e., when our OOTB transforms are detected by the tool).

[gharris1727]

We may choose to report broken plugin locations (i.e., any that has one or more unloadable plugins), but IMO it's better to leave this info out entirely. We have room to maneuver in the future with this tool if we want (the public interface boundaries set by the KIP are very permissive), and I'd rather we err on the side of simplicity for now. Total/loadable/compatible plugins seems like plenty.

I removed the location summaries and overall summaries. Users will need to inspect the table to figure out if everything is compatible, or run a worker verification, which is the more reliable way to find incompatibilities anyway.

I'm also worried about our alias collision reporting. Right now every invocation of the command so far has produced a ton of warnings about SMT aliases collisiding. It'd be nice if we could use ReplaceField$Key as an alias instead of just Key, but until/unless that KIP gets approved, we're stuck with a fairly large number of collisions with the aliases for our OOTB transforms.

I think this is the right call, since we already changed it so that the aliases in each row of the table doesn't depend on other rows. The alias collision logic was a red-herring because I accidentally used the collision logic in the original implementation, when it really isn't relevant to the tool at all.

[C0urante]

Thanks Greg, LGTM! (Pending CI build)

[gharris1727]

CI failures appear unrelated, and the tests pass locally.

[nizhikov]

Hello, @gharris1727 , @C0urante
I'm working on #13247 and this commit somehow interfere with my change :)
ReassignPartitionsIntegrationTest that I rewrite in java works before this commit and hangs after.
Meanwhile I don't find a reason - why this breaks my test, but removing all connect dependencies from classpath of tools makes test pass without any additional changes.

Do you have any idea - how connect:api dependency can affect test cluster or similar code?

[nizhikov]

In case someone interested, adding :connect:api dependency to tools somehow make :storage:api classes invisible.
I fix it adding explicit dependency on :storage:api project.