C0urante commented on code in PR #13821:
URL: https://github.com/apache/kafka/pull/13821#discussion_r1246751378
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java:
##########
@@ -104,6 +81,33 @@ public DelegatingClassLoader(List<Path> pluginLocations) {
this(pluginLocations, DelegatingClassLoader.class.getClassLoader());
}
+ public Set<PluginSource> sources() {
Review Comment:
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`.
--
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
