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
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`.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java:
##########
@@ -325,6 +328,34 @@ public static List<Path> pluginUrls(Path topPath) throws
IOException {
return Arrays.asList(archives.toArray(new Path[0]));
}
+ public static Set<PluginSource> pluginSources(List<Path> pluginLocations,
DelegatingClassLoader classLoader, ClassLoaderFactory factory) {
Review Comment:
Any reason not to generalize the signature, since we don't use any methods
specific to the `DelegatingClassLoader` class?
```suggestion
public static Set<PluginSource> pluginSources(List<Path>
pluginLocations, ClassLoader classLoader, ClassLoaderFactory factory) {
```
--
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
