Hi Greg, Thanks for the KIP! This part of Connect has been due for an update for a while, it's nice to see it getting some attention. I especially like how the migration plan is broken down into affected personas; makes review easier and, hopefully, makes the feature easier to understand for affected users.
Here are my initial thoughts: 1. Will workers' resiliency to packaging issues be affected when the new scanning behavior is enabled? For example, will a single poorly-packaged plugin prevent other correctly-packaged plugins from being loaded, or worse, cause the worker to fail? And either way, are there any details that we can call out to back up any guarantees we want to provide on that front? 2. I think the description of the "--plugin-location" flag may need some updating? In the middle of the list of bullet points there's "The value of this argument will be a single plugin, which can be any of the following:" but the following points don't appear to be related. 3. (Nit) Could we rename the migration script something like "connect-migrate-plugin-path" or even just "connect-plugin-path" with a "migrate" subcommand? 4. It's noted that "If a plugin class is removed from the path, the corresponding shim manifest should also be removed". Did you consider adding behavior (possibly guarded behind a CLI flag) to find and remove manifests for nonexistent plugins? 5. What's the concrete plan for updating the plugins that ship OOTB with Connect? Will they be given a service loader manifest, a module info file, or both? 6. In the HYBRID_WARN mode, what precedence will we give to plugins that are found by both the legacy scanning logic and the new service loader/module info loading logic? 7. It's a bit strange to immediately deprecate config property values and the migration script in their first release. IMO as long as we have the migration script in the project, it doesn't make sense to deprecate it, and until we've had at least one successful release with the new loading logic and some confidence in it, we should not deprecate the older logic. I think a more reasonable pace for the update here could be to deprecate the legacy scanning logic in 4.0 (changing the default accordingly) and remove it entirely in 5.0. Cheers, Chris On Mon, Jan 23, 2023 at 3:21 AM Federico Valeri <fedeval...@gmail.com> wrote: > Hi Greg, this looks like a useful change to me. > > I was just wondering if we should use a better script name like > "connect-convert-to-service-provider.sh" or something like this, and > maybe add a --dry-run option. > > On Tue, Jan 17, 2023 at 8:45 PM Greg Harris > <greg.har...@aiven.io.invalid> wrote: > > > > Hi all! > > > > I'd like to start a discussion about > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-898%3A+Modernize+Connect+plugin+discovery > > . > > > > Thanks! > > Greg Harris >
