You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rfscholte (via GitHub)" <gi...@apache.org> on 2024/08/29 09:51:29 UTC
[PR] Stp 3364 pinot plugin loader [pinot]
rfscholte opened a new pull request, #13907:
URL: https://github.com/apache/pinot/pull/13907
This PR must be merged after #13845 and it assumes that #13849 uses the same structure of the zipfile.
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte closed pull request #13907: pinot plugin loader
URL: https://github.com/apache/pinot/pull/13907
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "rfscholte (via GitHub)" <gi...@apache.org>.
rfscholte commented on PR #13907:
URL: https://github.com/apache/pinot/pull/13907#issuecomment-2326339893
Superseded by https://github.com/apache/pinot/pull/13930
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1741007304
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginsDirectoryVisitor.java:
##########
@@ -0,0 +1,45 @@
+package org.apache.pinot.spi.plugin;
+
+import java.io.IOException;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.Collection;
+import java.util.HashSet;
+
+
+class PluginsDirectoryVisitor extends SimpleFileVisitor<Path> {
Review 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.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1741001111
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginsDirectoryVisitor.java:
##########
@@ -0,0 +1,45 @@
+package org.apache.pinot.spi.plugin;
+
+import java.io.IOException;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.Collection;
+import java.util.HashSet;
+
+
+class PluginsDirectoryVisitor extends SimpleFileVisitor<Path> {
+
+ private final Collection<Path> _pluginDirs = new HashSet<>();
+
+ private final PinotPluginHandler _pinotPluginHandler;
+
+ private final ShadedPluginHandler _shadedPluginHandler;
+
+ public PluginsDirectoryVisitor(PinotPluginHandler _pinotPluginHandler, ShadedPluginHandler _shadedPluginHandler) {
+ this._pinotPluginHandler = _pinotPluginHandler;
+ this._shadedPluginHandler = _shadedPluginHandler;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
+ throws IOException {
+ if(_pinotPluginHandler.isPluginDirectory(file) || _shadedPluginHandler.isPluginDirectory(file)) {
+ _pluginDirs.add(file);
+ return FileVisitResult.CONTINUE;
+ }
+
+ return super.visitFile(file, attrs);
+ }
+
+ @Override
+ public FileVisitResult visitFileFailed(Path file, IOException exc)
+ throws IOException {
+ return super.visitFileFailed(file, exc);
+ }
Review Comment:
Why do we need to override this method?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1740986110
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PinotPluginHandler.java:
##########
@@ -0,0 +1,133 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.plugin;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.stream.Stream;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static java.util.function.Predicate.not;
+
+
+/**
+ * This handler assumes the following:
+ * Packed:
+ * [plugins-directory]/[plugin-name]/[plugin].zip
+ *
+ * Unpacked
+ * [plugins-directory]/[plugin-name]/classes/[**]/*.class
+ * [plugins-directory]/[plugin-name]/lib/*.jar
+ */
+class PinotPluginHandler {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotPluginHandler.class);
+
+ Collection<URL> toURLs(String pluginName, File directory) {
+ LOGGER.info("Trying to load plugin [{}] from location [{}]", pluginName, directory);
+
+ Collection<URL> urlList = new ArrayList<>();
+
+ // classes directory always has to go first
+ Path classes = directory.toPath().resolve("classes");
+ if (Files.isDirectory(classes)) {
+ try {
+ urlList.add(classes.toUri().toURL());
+ } catch (MalformedURLException e) {
+ LOGGER.error("Unable to load plugin [{}] classes directory", pluginName, e);
+ }
+ }
+
+ try (var stream = Files.newDirectoryStream(directory.toPath().resolve("lib"), e -> e.getFileName().toString().endsWith(".jar"))) {
Review Comment:
Bad news! We cannot use `var`, given we need to be compatible with Java 11
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1740992064
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PinotPluginHandler.java:
##########
@@ -0,0 +1,133 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.plugin;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.stream.Stream;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static java.util.function.Predicate.not;
+
+
+/**
+ * This handler assumes the following:
+ * Packed:
+ * [plugins-directory]/[plugin-name]/[plugin].zip
+ *
+ * Unpacked
+ * [plugins-directory]/[plugin-name]/classes/[**]/*.class
+ * [plugins-directory]/[plugin-name]/lib/*.jar
+ */
+class PinotPluginHandler {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(PinotPluginHandler.class);
+
+ Collection<URL> toURLs(String pluginName, File directory) {
+ LOGGER.info("Trying to load plugin [{}] from location [{}]", pluginName, directory);
+
+ Collection<URL> urlList = new ArrayList<>();
+
+ // classes directory always has to go first
+ Path classes = directory.toPath().resolve("classes");
+ if (Files.isDirectory(classes)) {
+ try {
+ urlList.add(classes.toUri().toURL());
+ } catch (MalformedURLException e) {
+ LOGGER.error("Unable to load plugin [{}] classes directory", pluginName, e);
+ }
+ }
+
+ try (var stream = Files.newDirectoryStream(directory.toPath().resolve("lib"), e -> e.getFileName().toString().endsWith(".jar"))) {
+ stream.forEach(f -> {
+ try {
+ urlList.add(f.toUri().toURL());
+ } catch (MalformedURLException e) {
+ LOGGER.error("Unable to load plugin [{}] jar file [{}]", pluginName, f.getFileName(), e);
+ }
+ });
+ } catch (IOException e) {
+ LOGGER.error("Unable to load plugin [{}]", pluginName, e);
+ }
+ return urlList;
+ }
+
+ boolean isPluginDirectory(Path p) {
Review 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.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1741007304
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginsDirectoryVisitor.java:
##########
@@ -0,0 +1,45 @@
+package org.apache.pinot.spi.plugin;
+
+import java.io.IOException;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.Collection;
+import java.util.HashSet;
+
+
+class PluginsDirectoryVisitor extends SimpleFileVisitor<Path> {
Review 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)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1740998881
##########
pinot-spi/src/test/java/org/apache/pinot/spi/plugin/PluginsDirectoryVisitorTest.java:
##########
@@ -0,0 +1,33 @@
+package org.apache.pinot.spi.plugin;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Set;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+
+public class PluginsDirectoryVisitorTest {
+
+ private final PinotPluginHandler _pinotPluginHandler = new PinotPluginHandler();
+ private final ShadedPluginHandler _shadedPluginHandler = new ShadedPluginHandler();
+
+ @Test
+ public void testVisitDirectory() {
+ PluginsDirectoryVisitor visitor = new PluginsDirectoryVisitor(_pinotPluginHandler, _shadedPluginHandler);
+
+ Path pluginsDirectory = Path.of("src/test/resources/plugins-directory");
+ try {
+ Files.walkFileTree(pluginsDirectory, Set.of(), 1, visitor);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
+ assertEquals(visitor.getPluginDirs().size(), 3);
+ assertTrue(visitor.getPluginDirs().contains(Path.of("src/test/resources/plugins-directory/shadedplugin-1")));
+ 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")));
+ }
+}
Review Comment:
Probably checkstyle will complain asking for a new EOL
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] pinot plugin loader [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13907:
URL: https://github.com/apache/pinot/pull/13907#discussion_r1741009065
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginsDirectoryVisitor.java:
##########
@@ -0,0 +1,45 @@
+package org.apache.pinot.spi.plugin;
+
+import java.io.IOException;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.Collection;
+import java.util.HashSet;
+
+
+class PluginsDirectoryVisitor extends SimpleFileVisitor<Path> {
+
+ private final Collection<Path> _pluginDirs = new HashSet<>();
+
+ private final PinotPluginHandler _pinotPluginHandler;
+
+ private final ShadedPluginHandler _shadedPluginHandler;
+
+ public PluginsDirectoryVisitor(PinotPluginHandler _pinotPluginHandler, ShadedPluginHandler _shadedPluginHandler) {
+ this._pinotPluginHandler = _pinotPluginHandler;
+ this._shadedPluginHandler = _shadedPluginHandler;
+ }
Review 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
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org