KAFKA-15069: Refactor plugin scanning logic into ReflectionScanner

[gharris1727]

In order to support multiple scanning modes, we should refactor the existing scanning mechanism out of the DelegatingClassLoader. This is because KIP-898 will require more functionality that relies on the results of scanning, and it is not appropriate to add this functionality to the DCL itself.

Scanning (and the PluginScanResult) are dependent on the ClassLoader instances which are used to load the plugins, so the protected functions for instantiating classloaders is moved to from Plugins and DelegatingClassLoader to the new PluginClassLoaderFactor and ClassLoaderFactory classes. The setup logic that constructs PluginClassLoaders for each pluginLocation is moved to PluginUtils.pluginSources.

In addition to pulling the existing reflection-based scanning out into a ReflectionScanner and superclass PluginScanner, rename DelegatingClassLoaderTest to PluginScannerTest, because it was primarily testing the plugin scanning.

This test will be expanded in a follow-up to be parameterized, and test the ServiceLoaderScanner in similar fashion to the ReflectionScanner.

To replace DelegatingClassLoaderTest, write a new test targeting the now much smaller scope of the DelegatingClassLoader, using mocked parent and plugin classloaders, as well as mocked plugin scanning logic.

Committer Checklist (excluded from commit message)

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

[C0urante]

@gharris1727 can we start filing Jira tickets for these changes to classloading logic? They're large enough that I don't think MINOR is warranted anymore, and it'd be nice to have a picture of how they fit into the high-level implementation plan for KIP-898.

[C0urante]

Thanks Greg! I really like the shape of this and think I can see how this lays the groundwork for KIP-898 better. Left a few questions/thoughts.

Also, even though it's unused in this patch, if we're going to add the ServiceLoaderScanner class, we should also add tests for it. I'd also be fine if we left that class out for now and introduced it in a subsequent PR that starts to make use of it.

[C0urante]

Any reason not to generalize the signature, since we don't use any methods specific to the DelegatingClassLoader class?

Suggested change

	public static Set<PluginSource> pluginSources(List<Path> pluginLocations, DelegatingClassLoader classLoader, ClassLoaderFactory factory) {
	public static Set<PluginSource> pluginSources(List<Path> pluginLocations, ClassLoader classLoader, ClassLoaderFactory factory) {

[C0urante]

I think it's fine in PluginUtils if we plan on using it in multiple places. Otherwise, we may want to downgrade its visibility and move it into the single class that uses it. Either way, thanks for the change--I think this is an improvement 👍

I'm not a huge fan of how we're using the ClassLoaderFactory class right now, though. I can see the value for it in the Plugins class when both methods are used, but requiring an instance of it in PluginUtils::pluginSources seems like overkill since we don't need access to the newDelegatingClassLoader method.

Could we create a separate PluginClassLoaderFactory interface, have ClassLoaderFactory implement that interface, and change the signature of PluginUtils::pluginSources to accept an instance of that interface instead of a ClassLoaderFactory?

Also, it may be a little unclear to first-time readers why we have the separate ClassLoaderFactory class since it tracks no (instance or static) state and seems at first glance like all of its logic might be a better fit for the PluginUtils class. Can we document in that class that its purpose is to provide a layer of indirection for the purpose of easier mocking in tests?

Finally, we don't have to copy the visibility of the methods that we've extracted into the ClassLoaderFactory class; IMO both of those can and should be made public.

[C0urante]

Thanks Greg! The latest changes look great. A few small comments left about some of the internal APIs we're introducing here. We also still need to either add tests for the service loader scanner, or remove that class from this PR.

[gharris1727]

@C0urante Thanks for the helpful comments! The initial version was just-enough to get later stuff working, but the latest version is a much more complete refactor.

We also still need to either add tests for the service loader scanner, or remove that class from this PR.

To properly test the ServiceLoaderScanner, I'll need to add manifests to the TestPlugins. To avoid scope creep, I've moved the ServiceLoaderScanner out of this PR. I'll add it in a follow-up along with the necessary test changes.

This is ready for another pass.

[C0urante]

LGTM! Thanks Greg