pinot plugin loader #13907
pinot plugin loader #13907
Conversation
…here should be different configuration for this instance
| } | ||
| } | ||
|
|
||
| try (var stream = Files.newDirectoryStream(directory.toPath().resolve("lib"), e -> e.getFileName().toString().endsWith(".jar"))) { |
There was a problem hiding this comment.
Bad news! We cannot use var, given we need to be compatible with Java 11
| return urlList; | ||
| } | ||
|
|
||
| boolean isPluginDirectory(Path p) { |
There was a problem hiding this comment.
Can we add some javadocs here?
Also I'm not sure about the method naming. From the caller's perspective a method called isPluginDirectory looks like something cheap and without side effects, while this method is actually unziping a zip file.
| assertTrue(visitor.getPluginDirs().contains(Path.of("src/test/resources/plugins-directory/pinotplugin-1"))); | ||
| assertTrue(visitor.getPluginDirs().contains(Path.of("src/test/resources/plugins-directory/pinotplugin-2"))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Probably checkstyle will complain asking for a new EOL
| @Override | ||
| public FileVisitResult visitFileFailed(Path file, IOException exc) | ||
| throws IOException { | ||
| return super.visitFileFailed(file, exc); | ||
| } |
There was a problem hiding this comment.
Why do we need to override this method?
| import java.util.HashSet; | ||
|
|
||
|
|
||
| class PluginsDirectoryVisitor extends SimpleFileVisitor<Path> { |
There was a problem hiding this comment.
nit: I don't like having these individual classes that are designed to resolve a problem that is specific to some other class. It is not important, but I would prefer to refactor this class as a inner class of PluginManager.
You would be able then to remove the two attributes (and therefore the constructor)
| public PluginsDirectoryVisitor(PinotPluginHandler _pinotPluginHandler, ShadedPluginHandler _shadedPluginHandler) { | ||
| this._pinotPluginHandler = _pinotPluginHandler; | ||
| this._shadedPluginHandler = _shadedPluginHandler; | ||
| } |
There was a problem hiding this comment.
the code style checker is going to complain about that. Attributes must be prefixed with _ and variables must not be prefixed with _. Also this.attribute is forbidden.
I hate that code style but... one gets used to it :D
|
Superseded by #13930 |
This PR must be merged after #13845 and it assumes that #13849 uses the same structure of the zipfile.