KAFKA-15031: Add plugin.discovery to Connect worker configuration (KIP-898)

[gharris1727]

This is the primary feature for KIP-898, which allows users to reconfigure the Connect worker among the different scanning modes. The different modes and their functionality and intended use are as described in https://cwiki.apache.org/confluence/display/KAFKA/KIP-898%3A+Modernize+Connect+plugin+discovery

In addition to the configuration defaulting to HYBRID_WARN for production use-cases, the default for the EmbeddedConnectCluster is set to HYBRID_FAIL. This has necessitated that all of the connect:runtime:main and connect:runtime:test plugins get valid manifest files so that they can be loaded. This includes the Mock, Schema, and Verifiable connectors that are being relocated in #13302, so these PRs will conflict.

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 an example error log output when in HYBRID_FAIL mode (I shortened the listing for brevity; the actual prints include every non-migrated plugin)

Plugins are missing ServiceLoader manifests, these plugins will not be visible with plugin.discovery=SERVICE_LOAD: [
classpath	org.apache.kafka.connect.converters.ByteArrayConverter	undefined
classpath	org.apache.kafka.connect.converters.DoubleConverter	undefined
classpath	org.apache.kafka.connect.converters.FloatConverter	undefined
classpath	org.apache.kafka.connect.converters.IntegerConverter	undefined
classpath	org.apache.kafka.connect.converters.LongConverter	undefined
classpath	org.apache.kafka.connect.converters.ShortConverter	undefined
classpath	org.apache.kafka.connect.integration.BlockingConnectorTest$BlockingConnector	0.0.0
classpath	org.apache.kafka.connect.integration.BlockingConnectorTest$BlockingSinkConnector	0.0.0
...
classpath	org.apache.kafka.connect.transforms.predicates.TopicNameMatches	undefined
]
	at app//org.apache.kafka.connect.runtime.isolation.Plugins.maybeReportHybridDiscoveryIssue(Plugins.java:131)
	at app//org.apache.kafka.connect.runtime.isolation.Plugins.initLoaders(Plugins.java:92)
	at app//org.apache.kafka.connect.runtime.isolation.Plugins.<init>(Plugins.java:74)
	at app//org.apache.kafka.connect.runtime.isolation.Plugins.<init>(Plugins.java:64)
	at app//org.apache.kafka.connect.cli.AbstractConnectCli.startConnect(AbstractConnectCli.java:121)
	at app//org.apache.kafka.connect.util.clusters.WorkerHandle.start(WorkerHandle.java:50)
	at app//org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster.addWorker(EmbeddedConnectCluster.java:187)
	at app//org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster.startConnect(EmbeddedConnectCluster.java:283)
	at app//org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster.start(EmbeddedConnectCluster.java:148)
	...
	(test suite)

This is what appears if the manifests included in this PR are not present.

The same, but with HYBRID_WARN:

[2023-07-19 17:03:44,143] WARN Plugins are missing ServiceLoader manifests, these plugins will not be visible with plugin.discovery=SERVICE_LOAD: [
classpath	org.apache.kafka.connect.converters.ByteArrayConverter	undefined
...
classpath	org.apache.kafka.connect.transforms.predicates.TopicNameMatches	undefined
] (org.apache.kafka.connect.runtime.isolation.Plugins:128)

I did notice that if logging is OFF or ERROR for the runtime (such as in the mirror tests) then the warning doesn't print. The exception does propagate, so they will certainly see the message in HYBRID_FAIL.

I'm going to push the required manifests, so just delete/revert them if you'd like to see these errors on the classpath.

[C0urante]

Thanks Greg! Lots of comments but most are fairly small.

The question of plugins that are installed directly onto the classpath might be a bit tricky... let me know if I've missed something. My current understanding is that these plugins will continue to be available on the worker regardless of which discovery mode is used (with the possible exception of HYBRID_FAIL, if they prevent workers from starting up), but that they may not be visible in portions of the REST API (I'm thinking GET /connector-plugins, though there may be others).

[C0urante]

Nit: language

Suggested change

	"Plugins are missing ServiceLoader manifests, these plugins will not be visible with %s=%s: %s",
	"One or more plugins are missing ServiceLoader manifests and will not be visible with %s=%s: %s",

[C0urante]

Also, should we include instructions on what users should do in this case? For many of them, this is going to be the first time hearing about new discovery logic; we should make this message as informative as possible if we want them to start to make use of it.

Some options include:

• Suggest that they add service loader manifests for these plugins (possibly by mentioning the CLI tool added in KAFKA-15030: Add connect-plugin-path command-line tool. #14064)
• Link to a docs section on the website describing the new plugin discovery logic
• Link to a docs section for the Java standard library describing the service loader feature
• In the case of HYBRID_FAIL, suggest changing the discovery mode if the missing plugins cannot be updated

[C0urante]

Also also, if the point about classpath-installed plugins being unaffected by the plugin discovery mode is correct, should we either filter these plugins out from missingPlugins or clarify that those plugins will still be usable with SERVICE_LOAD?

I suppose "visible" is accurate in the sense that these plugins won't be listed by the GET /connector-plugins endpoint, but that may confuse users if they notice that they're still able to use these plugins in a connector config after switching to SERVICE_LOAD.

[gharris1727]

For many of them, this is going to be the first time hearing about new discovery logic; we should make this message as informative as possible if we want them to start to make use of it.
Link to a docs section on the website describing the new plugin discovery logic

I think to explaining everything in one warning/error message is unreasonable, and linking out to external documentation is the better strategy. The error message is already lengthy enough with each plugin listing.

[gharris1727]

Linked documentation is added in #14068

[C0urante]

Looks great--one more thing: can we make the same tweak to the language here that we did in WorkerConfig by replacing "will not be visible" with "may not be usable"?

[gharris1727]

The question of plugins that are installed directly onto the classpath might be a bit tricky... let me know if I've missed something. My current understanding is that these plugins will continue to be available on the worker regardless of which discovery mode is used (with the possible exception of HYBRID_FAIL, if they prevent workers from starting up), but that they may not be visible in portions of the REST API (I'm thinking GET /connector-plugins, though there may be others).

This is incorrect, and I think i've contributed to the confusion, sorry. To be very explicit:

1. The plugin.discovery configuration treats classpath and plugin.path the same. If a non-migrated plugin is on the classpath, then it will appear in HYBRID_WARN warnings, HYBRID_FAIL exceptions, and won't be usable with SERVICE_LOAD. It will not appear in GET /connector-plugins and attempting to use it will cause ClassNotFound or similar exceptions to be thrown.
2. The connect-plugin-path script will only migrate plugins included in it's arguments, leaving classpath completely unchanged. The list command will not show classpath plugins. This means someone may have HYBRID_FAIL enabled, and the connect-plugin-path script will not solve it, such as in unit tests, when everything is on the classpath.
3. If someone adds non-migrated plugins to the classpath (such as in a production deployment), they will need to pay additional attention to those plugins. They can either move them to plugin.path, upgrade them, or use --plugin-location to specify each one manually.

[gharris1727]

The question of plugins that are installed directly onto the classpath might be a bit tricky... let me know if I've missed something. My current understanding is that these plugins will continue to be available on the worker regardless of which discovery mode is used (with the possible exception of HYBRID_FAIL, if they prevent workers from starting up), but that they may not be visible in portions of the REST API (I'm thinking GET /connector-plugins, though there may be others).

This is incorrect, and I think i've contributed to the confusion, sorry.

I apologize, you are correct, and I was the one who was confused. Because someone can still specify the classname directly, and that classname is loadable in the AppClassLoader, then the plugin will still be "usable", but aliases will be missing and it will be hidden from the REST API.

I don't think it's possible to make the classpath and plugins behave the consistently in this case. If SERVICE_LOAD doesn't discover the plugin, then it's indistinguishable from a non-plugin in the DelegatingClassLoader#loadClass. And we have to allow non-plugins to delegate upwards, because that's how every other class is loaded. We would make a situation where the runtime can't load a classpath plugin but the plugins can, which is a bit silly. Do you think this behavior is worth changing?

I've updated the documentation string (and site docs) to be intentionally vague and say "may not be usable" to cover both cases, let me know if you'd like to adjust that.

Also just small style choice: Do you think we should use all-caps or lowercase values for this configuration in docs?

[C0urante]

I don't think it's possible to make the classpath and plugins behave the consistently in this case... Do you think this behavior is worth changing?

Absolutely not! Haha, I'm mostly just concerned with our language being accurate. The couching in place here ("may not be usable") looks fine to me. We could theoretically catch cases where we've loaded a connector from the classpath after it's been loaded and then throw an exception, but I don't think the consistency this would bring outweighs the cost in UX.

Also just small style choice: Do you think we should use all-caps or lowercase values for this configuration in docs?

I think we tend to do lowercase; see exactly.once.source.support and isolation.level for a couple examples.

[C0urante]

LGTM assuming the two small remaining comments (tweaking language in Plugins::maybeReportHybridDiscoveryIssue and using lowercase values in docs for plugin.discovery.mode property) are addressed. Thanks Greg!

[C0urante]

Ah, spoke too soon--it looks like the SynchronizationTest::testSimultaneousUpwardAndDownwardDelegating test case is consistently failing now. Can we look into that before merging?

[gharris1727]

LGTM assuming the two small remaining comments (tweaking language in Plugins::maybeReportHybridDiscoveryIssue and using lowercase values in docs for plugin.discovery.mode property) are addressed. Thanks Greg!

These have been addressed.

Ah, spoke too soon--it looks like the SynchronizationTest::testSimultaneousUpwardAndDownwardDelegating test case is consistently failing now. Can we look into that before merging?

The test was failing because the ServiceLoaderScanner called PluginClassLoader#loadClass on the JsonConverter during the Plugins initialization. This caused the PluginClassLoader to cache the class, preventing the test from causing an upward-delegation. I've swapped out the JsonConverter for an arbitrary non-plugin class that isn't loaded during initialization.

[C0urante]

Thanks Greg! LGTM pending CI build

[gharris1727]

Flaky test failures appear unrelated, and the tests pass locally.