You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/12/06 17:12:30 UTC
[GitHub] [pinot] priyen opened a new pull request #7871: Support loading plugins from directories
priyen opened a new pull request #7871:
URL: https://github.com/apache/pinot/pull/7871
## Description
This change allows Pinot to load plugins from multiple directories. The existing plugin property name, ```PLUGINS_DIR_PROPERTY_NAME = "plugins.dir"```, originally is a string but now is a semi-colon delimited string which can have multiple paths.
-->
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
- no it does not
Does this PR fix a zero-downtime upgrade introduced earlier?
- n/a
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765202430
##########
File path: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java
##########
@@ -390,27 +391,41 @@ protected void addMapperJarToDistributedCache(Job job, PinotFS outputDirFS, URI
}
protected void packPluginsToDistributedCache(Job job, PinotFS outputDirFS, URI stagingDirURI) {
- File pluginsRootDir = new File(PluginManager.get().getPluginsRootDir());
- if (pluginsRootDir.exists()) {
- try {
- File pluginsTarGzFile = File.createTempFile("pinot-plugins-", ".tar.gz");
- TarGzCompressionUtils.createTarGzFile(pluginsRootDir, pluginsTarGzFile);
-
- // Copy to staging directory
- Path cachedPluginsTarball = new Path(stagingDirURI.toString(), SegmentGenerationUtils.PINOT_PLUGINS_TAR_GZ);
- outputDirFS.copyFromLocalFile(pluginsTarGzFile, cachedPluginsTarball.toUri());
- job.addCacheFile(cachedPluginsTarball.toUri());
- } catch (Exception e) {
- LOGGER.error("Failed to tar plugins directory and upload to staging dir", e);
- throw new RuntimeException(e);
- }
+ String[] pluginDirectories = PluginManager.get().getPluginsDirectories();
+ if (pluginDirectories == null) {
+ LOGGER.warn("Plugin directories is null, nothing to pack to distributed cache");
+ return;
+ }
- String pluginsIncludes = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
- if (pluginsIncludes != null) {
- job.getConfiguration().set(PLUGINS_INCLUDE_PROPERTY_NAME, pluginsIncludes);
+ ArrayList<File> validPluginDirectories = new ArrayList();
+
+ for (String pluginsDirPath : pluginDirectories) {
+ File pluginsDir = new File(pluginsDirPath);
+ if (pluginsDir.exists()) {
+ validPluginDirectories.add(pluginsDir);
+ } else {
+ LOGGER.warn("Cannot find Pinot plugins directory at [{}]", pluginsDirPath);
+ return;
}
- } else {
Review comment:
Replaced it with the earlier return at ln 408 if we found a directory that doesn't exist.
Previous behaviour was to do nothing but log, so I think in this case where we have a list of directories, it's better to not do anything if one of them is invalid
--
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
[GitHub] [pinot] xiangfu0 commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765326046
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,15 +56,27 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[] {inputFile}, outputFile);
+ }
+
+ /**
+ * Creates a tar.gz file from a list of input file/directories to the output file. The output file must have
+ * ".tar.gz" as the file extension.
+ */
+ public static void createTarGzFile(File[] inputFiles, File outputFile)
+ throws IOException {
Preconditions.checkArgument(outputFile.getName().endsWith(TAR_GZ_FILE_EXTENSION),
"Output file: %s does not have '.tar.gz' file extension", outputFile);
try (OutputStream fileOut = Files.newOutputStream(outputFile.toPath());
- BufferedOutputStream bufferedOut = new BufferedOutputStream(fileOut);
- OutputStream gzipOut = new GzipCompressorOutputStream(bufferedOut);
- TarArchiveOutputStream tarGzOut = new TarArchiveOutputStream(gzipOut)) {
+ BufferedOutputStream bufferedOut = new BufferedOutputStream(fileOut);
Review comment:
fix the indentation?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,6 +56,15 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[]{inputFile}, outputFile);
+ }
+
+ /**
+ * Creates a tar.gz file from a list of input file/directories to the output file. The output file must have
+ * ".tar.gz" as the file extension.
+ */
+ public static void createTarGzFile(File[] inputFiles, File outputFile)
Review comment:
shall we check and skip the nested paths?
E.g. one directory is `a/b/c` and one file `a/b/c/d.file`
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
Right now Pinot uses `appAssemblerScriptTemplate` to set all the plugins into java classpath.
So far let's use semi-colon to make the delimiter.
For PluginManager.java, we should also follow the same delimiter convention to use semi-colon. For `pluginInclude`, we can make the colon as backward compatible.
--
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
[GitHub] [pinot] jadami10 commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
jadami10 commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r764179452
##########
File path: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java
##########
@@ -390,27 +391,41 @@ protected void addMapperJarToDistributedCache(Job job, PinotFS outputDirFS, URI
}
protected void packPluginsToDistributedCache(Job job, PinotFS outputDirFS, URI stagingDirURI) {
- File pluginsRootDir = new File(PluginManager.get().getPluginsRootDir());
- if (pluginsRootDir.exists()) {
- try {
- File pluginsTarGzFile = File.createTempFile("pinot-plugins-", ".tar.gz");
- TarGzCompressionUtils.createTarGzFile(pluginsRootDir, pluginsTarGzFile);
-
- // Copy to staging directory
- Path cachedPluginsTarball = new Path(stagingDirURI.toString(), SegmentGenerationUtils.PINOT_PLUGINS_TAR_GZ);
- outputDirFS.copyFromLocalFile(pluginsTarGzFile, cachedPluginsTarball.toUri());
- job.addCacheFile(cachedPluginsTarball.toUri());
- } catch (Exception e) {
- LOGGER.error("Failed to tar plugins directory and upload to staging dir", e);
- throw new RuntimeException(e);
- }
+ String[] pluginDirectories = PluginManager.get().getPluginsDirectories();
+ if (pluginDirectories == null) {
+ LOGGER.warn("Plugin directories is null, nothing to pack to distributed cache");
+ return;
+ }
- String pluginsIncludes = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
- if (pluginsIncludes != null) {
- job.getConfiguration().set(PLUGINS_INCLUDE_PROPERTY_NAME, pluginsIncludes);
+ ArrayList<File> validPluginDirectories = new ArrayList();
+
+ for (String pluginsDirPath : pluginDirectories) {
+ File pluginsDir = new File(pluginsDirPath);
+ if (pluginsDir.exists()) {
+ validPluginDirectories.add(pluginsDir);
+ } else {
+ LOGGER.warn("Cannot find Pinot plugins directory at [{}]", pluginsDirPath);
+ return;
}
- } else {
- LOGGER.warn("Cannot find local Pinot plugins directory at [{}]", pluginsRootDir);
+ }
+
+ File pluginsTarGzFile = new File(PINOT_PLUGINS_TAR_GZ);
+ try {
+ File[] files = validPluginDirectories.toArray(new File[0]);
+ TarGzCompressionUtils.createTarGzFile(files, pluginsTarGzFile);
Review comment:
how does this untar later? If I have
```
/a/b
/a/c
/b/c
```
will it come back out the same way?
##########
File path: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java
##########
@@ -390,27 +391,41 @@ protected void addMapperJarToDistributedCache(Job job, PinotFS outputDirFS, URI
}
protected void packPluginsToDistributedCache(Job job, PinotFS outputDirFS, URI stagingDirURI) {
- File pluginsRootDir = new File(PluginManager.get().getPluginsRootDir());
- if (pluginsRootDir.exists()) {
- try {
- File pluginsTarGzFile = File.createTempFile("pinot-plugins-", ".tar.gz");
- TarGzCompressionUtils.createTarGzFile(pluginsRootDir, pluginsTarGzFile);
-
- // Copy to staging directory
- Path cachedPluginsTarball = new Path(stagingDirURI.toString(), SegmentGenerationUtils.PINOT_PLUGINS_TAR_GZ);
- outputDirFS.copyFromLocalFile(pluginsTarGzFile, cachedPluginsTarball.toUri());
- job.addCacheFile(cachedPluginsTarball.toUri());
- } catch (Exception e) {
- LOGGER.error("Failed to tar plugins directory and upload to staging dir", e);
- throw new RuntimeException(e);
- }
+ String[] pluginDirectories = PluginManager.get().getPluginsDirectories();
+ if (pluginDirectories == null) {
+ LOGGER.warn("Plugin directories is null, nothing to pack to distributed cache");
+ return;
+ }
- String pluginsIncludes = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
- if (pluginsIncludes != null) {
- job.getConfiguration().set(PLUGINS_INCLUDE_PROPERTY_NAME, pluginsIncludes);
+ ArrayList<File> validPluginDirectories = new ArrayList();
+
+ for (String pluginsDirPath : pluginDirectories) {
+ File pluginsDir = new File(pluginsDirPath);
+ if (pluginsDir.exists()) {
+ validPluginDirectories.add(pluginsDir);
+ } else {
+ LOGGER.warn("Cannot find Pinot plugins directory at [{}]", pluginsDirPath);
+ return;
}
- } else {
Review comment:
why remove this?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,55 +124,66 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
- }
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
- }
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
+ String[] directories = pluginsDirectories.split(";");
+ LOGGER.info("Plugin directories env: {}, parsed directories to load: '{}'", pluginsDirectories, directories);
+
+ for (String pluginsDirectory : directories) {
+ if (!new File(pluginsDirectory).exists()) {
+ LOGGER.warn("Plugins dir [{}] doesn't exist.", pluginsDirectory);
+ return;
+ }
+
+ Collection<File> jarFiles = FileUtils.listFiles(
+ new File(pluginsDirectory),
+ new String[]{JAR_FILE_EXTENSION},
+ true
+ );
+ List<String> pluginsToLoad = null;
+ if (!StringUtils.isEmpty(pluginsInclude)) {
+ pluginsToLoad = Arrays.asList(pluginsInclude.split(";"));
Review comment:
what happens when you accidentally have a plugin with the same name in 2 directories? My guess from reading the code is the second one actually applies. I guess the other option is to say "plugin already loaded, skipping". Either way, I think we should explicitly codify it and add a test.
--
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
[GitHub] [pinot] priyen-stripe commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen-stripe commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r768980819
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,6 +56,15 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[]{inputFile}, outputFile);
+ }
+
+ /**
+ * Creates a tar.gz file from a list of input file/directories to the output file. The output file must have
+ * ".tar.gz" as the file extension.
+ */
+ public static void createTarGzFile(File[] inputFiles, File outputFile)
Review comment:
@xiangfu0 can you clarify what you mean? so in that example `a/b/c/d.file` would be skipped since it's already included in `a/b/c`?
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r768981652
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,6 +56,15 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[]{inputFile}, outputFile);
+ }
+
+ /**
+ * Creates a tar.gz file from a list of input file/directories to the output file. The output file must have
+ * ".tar.gz" as the file extension.
+ */
+ public static void createTarGzFile(File[] inputFiles, File outputFile)
Review comment:
@xiangfu0 can you clarify what you mean, so `a/b/c/d.file` would be skipped since it's included already via `a/b/c`?
--
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
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765384636
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
@xiangfu0 Can you please take a look?
--
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
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765365466
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
I just realized that the `pluginsInclude` separator is changed from ',' to ';' in this PR which can cause backward incompatibility. Does it make sense to allow both as separator?
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (166d330) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `40.64%`.
> The diff coverage is `18.42%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
=============================================
- Coverage 71.65% 31.01% -40.65%
=============================================
Files 1581 1572 -9
Lines 81350 81015 -335
Branches 12128 12093 -35
=============================================
- Hits 58293 25128 -33165
- Misses 19117 53712 +34595
+ Partials 3940 2175 -1765
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `29.42% <18.42%> (+0.10%)` | :arrow_up: |
| integration2 | `27.91% <18.42%> (+0.10%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-48.56%)` | :arrow_down: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `78.87% <100.00%> (-6.43%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/readers/FileFormat.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9yZWFkZXJzL0ZpbGVGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1078 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...166d330](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (541911a) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `6.56%`.
> The diff coverage is `22.50%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
============================================
- Coverage 71.65% 65.09% -6.57%
- Complexity 4080 4082 +2
============================================
Files 1581 1538 -43
Lines 81350 80002 -1348
Branches 12128 12037 -91
============================================
- Hits 58293 52077 -6216
- Misses 19117 24202 +5085
+ Partials 3940 3723 -217
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `68.45% <22.50%> (-0.35%)` | :arrow_down: |
| unittests2 | `14.44% <0.00%> (-0.07%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `46.52% <6.06%> (-2.03%)` | :arrow_down: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `85.91% <100.00%> (+0.62%)` | :arrow_up: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...he/pinot/common/messages/SegmentReloadMessage.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvU2VnbWVudFJlbG9hZE1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/core/data/manager/realtime/TimerService.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvVGltZXJTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...not/common/exception/HttpErrorStatusException.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL0h0dHBFcnJvclN0YXR1c0V4Y2VwdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [364 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...541911a](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765347562
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
I used `;` as the plugin directories include property also used it to be consistent, but I am fine with changing it to `,`, what do you think?
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (64d8672) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `40.69%`.
> The diff coverage is `25.00%`.
> :exclamation: Current head 64d8672 differs from pull request most recent head f32149c. Consider uploading reports for the commit f32149c to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
=============================================
- Coverage 71.65% 30.96% -40.70%
=============================================
Files 1581 1572 -9
Lines 81350 81015 -335
Branches 12128 12093 -35
=============================================
- Hits 58293 25085 -33208
- Misses 19117 53762 +34645
+ Partials 3940 2168 -1772
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `29.40% <25.00%> (+0.09%)` | :arrow_up: |
| integration2 | `27.91% <25.00%> (+0.11%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-48.56%)` | :arrow_down: |
| [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `72.59% <66.66%> (-19.36%)` | :arrow_down: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `78.87% <100.00%> (-6.43%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1078 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...f32149c](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] jackjlli commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r764280754
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,15 +56,23 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[] {inputFile}, outputFile);
+ }
+
+ public static void createTarGzFile(File[] inputFiles, File outputFile)
Review comment:
Could you add the javadoc for this method in order to differentiate with the above one?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -338,4 +352,4 @@ public void registerRecordReaderClass(String inputFormat, String recordReaderCla
INPUT_FORMAT_TO_RECORD_READER_CONFIG_CLASS_NAME_MAP.put(inputFormat.toLowerCase(), recordReaderConfigClass);
}
}
-}
+}
Review comment:
Missing empty tail line.
##########
File path: pinot-tools/src/main/resources/appAssemblerScriptTemplate
##########
@@ -109,35 +109,41 @@ fi
# Set $PLUGINS_CLASSPATH for plugin jars to be put into classpath.
# $PLUGINS_DIR and $PLUGINS_INCLUDE are used if $PLUGINS_CLASSPATH is not set.
-# $PLUGINS_DIR is the root directory of plugins directory, default to '"$BASEDIR"/plugins' if not set.
+# $PLUGINS_DIR is semi-colon separated list of plugin directories, default to '"$BASEDIR"/plugins' if not set.
# $PLUGINS_INCLUDE is semi-colon separated plugins name, e.g. pinot-avro;pinot-batch-ingestion-standalone. Default is not set, which means load all the plugin jars.
if [ -z "$PLUGINS_CLASSPATH" ] ; then
if [ -z "$PLUGINS_DIR" ] ; then
PLUGINS_DIR="$BASEDIR"/plugins
fi
- if [ -d "$PLUGINS_DIR" ] ; then
- if [ -n "$PLUGINS_INCLUDE" ] ; then
- export IFS=";"
- for PLUGIN_JAR in $PLUGINS_INCLUDE; do
- PLUGIN_JAR_PATH=$(find "$PLUGINS_DIR" -path \*/"$PLUGIN_JAR"/"$PLUGIN_JAR"-\*.jar)
+
+ export IFS=';'
+ for DIR in $PLUGINS_DIR; do
+ if [ -d "$DIR" ] ; then
+ unset IFS
+ if [ -n "$PLUGINS_INCLUDE" ] ; then
+ export IFS=';'
+ for PLUGIN_JAR in $PLUGINS_INCLUDE; do
+ PLUGIN_JAR_PATH=$(find "$DIR" -path \*/"$PLUGIN_JAR"/"$PLUGIN_JAR"-\*.jar)
+ if [ -n "$PLUGINS_CLASSPATH" ] ; then
+ PLUGINS_CLASSPATH=$PLUGINS_CLASSPATH:$PLUGIN_JAR_PATH
Review comment:
I guess if there are two jars with the same jar name but in different directories are specified, the first one would be picked up. If that's the case, could you specify it in the above comments?
##########
File path: pinot-common/src/test/java/org/apache/pinot/common/utils/TarGzCompressionUtilsTest.java
##########
@@ -81,6 +81,48 @@ public void testFile()
assertEquals(FileUtils.readFileToString(untarredFile), fileContent);
}
+ @Test
+ public void testDirectories()
+ throws IOException {
+ String dirToTarName1 = "dir1";
+ String dirToTarName2 = "dir2";
+ File dir1 = new File(DATA_DIR, dirToTarName1);
+ File dir2 = new File(DATA_DIR, dirToTarName2);
+
+ File[] dirsToTar = new File[] {dir1, dir2};
+
+ String fileName1 = "data1";
+ String fileContent1 = "fileContent1";
+ String fileName2 = "data2";
+ String fileContent2 = "fileContent2";
+ FileUtils.write(new File(dir1, fileName1), fileContent1);
+ FileUtils.write(new File(dir2, fileName2), fileContent2);
+
+ String outputTarName = "output_tar" + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION;
+ File tarGzFile = new File(TAR_DIR, outputTarName);
+ TarGzCompressionUtils.createTarGzFile(dirsToTar, tarGzFile);
+
+ List<File> untarredFiles = TarGzCompressionUtils.untar(tarGzFile, UNTAR_DIR);
+ assertEquals(untarredFiles.size(), 4);
+
+ File untarredFileDir1 = untarredFiles.get(0);
+ File untarredFileDir2 = untarredFiles.get(2);
Review comment:
Please add some comments on why the 1st and 3rd would be fetched.
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765359378
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,15 +56,27 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[] {inputFile}, outputFile);
Review comment:
pushed commit and formated using the pinot style
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,15 +56,27 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[] {inputFile}, outputFile);
Review comment:
pushed commit and formatted using the pinot style
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765335042
##########
File path: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java
##########
@@ -390,27 +391,41 @@ protected void addMapperJarToDistributedCache(Job job, PinotFS outputDirFS, URI
}
protected void packPluginsToDistributedCache(Job job, PinotFS outputDirFS, URI stagingDirURI) {
- File pluginsRootDir = new File(PluginManager.get().getPluginsRootDir());
- if (pluginsRootDir.exists()) {
- try {
- File pluginsTarGzFile = File.createTempFile("pinot-plugins-", ".tar.gz");
- TarGzCompressionUtils.createTarGzFile(pluginsRootDir, pluginsTarGzFile);
-
- // Copy to staging directory
- Path cachedPluginsTarball = new Path(stagingDirURI.toString(), SegmentGenerationUtils.PINOT_PLUGINS_TAR_GZ);
- outputDirFS.copyFromLocalFile(pluginsTarGzFile, cachedPluginsTarball.toUri());
- job.addCacheFile(cachedPluginsTarball.toUri());
- } catch (Exception e) {
- LOGGER.error("Failed to tar plugins directory and upload to staging dir", e);
- throw new RuntimeException(e);
- }
+ String[] pluginDirectories = PluginManager.get().getPluginsDirectories();
+ if (pluginDirectories == null) {
+ LOGGER.warn("Plugin directories is null, nothing to pack to distributed cache");
+ return;
+ }
- String pluginsIncludes = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
- if (pluginsIncludes != null) {
- job.getConfiguration().set(PLUGINS_INCLUDE_PROPERTY_NAME, pluginsIncludes);
+ ArrayList<File> validPluginDirectories = new ArrayList();
+
+ for (String pluginsDirPath : pluginDirectories) {
+ File pluginsDir = new File(pluginsDirPath);
+ if (pluginsDir.exists()) {
+ validPluginDirectories.add(pluginsDir);
+ } else {
+ LOGGER.warn("Cannot find Pinot plugins directory at [{}]", pluginsDirPath);
+ return;
}
- } else {
- LOGGER.warn("Cannot find local Pinot plugins directory at [{}]", pluginsRootDir);
+ }
+
+ File pluginsTarGzFile = new File(PINOT_PLUGINS_TAR_GZ);
+ try {
+ File[] files = validPluginDirectories.toArray(new File[0]);
+ TarGzCompressionUtils.createTarGzFile(files, pluginsTarGzFile);
Review comment:
so the initial createTarGzFile which accepted 1 File obj, also accepted directories with support for recursion too: so let's say I called createTarGzFile with 1 directory (/a/...) that entire one will be tarred and it's children.
now, using the new method if I call it on let's say [ /a/ and /b/ ], the dir name is used as the baseEntryName (see ln 89 in TarGzCompressionUtils.java).
so effectively, if you tar two directories /a/ and /b/, it should come the same way as you pasted above
--
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
[GitHub] [pinot] xiangfu0 commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r768994534
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,6 +56,15 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[]{inputFile}, outputFile);
+ }
+
+ /**
+ * Creates a tar.gz file from a list of input file/directories to the output file. The output file must have
+ * ".tar.gz" as the file extension.
+ */
+ public static void createTarGzFile(File[] inputFiles, File outputFile)
Review comment:
yes, if the override happens then it's not a problem to worry.
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765378990
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
This is a bit odd, the master bash code in `pinot-tools/src/main/resources/appAssemblerScriptTemplate` is using `export IFS=";" when looping through `$PLUGINS_INCLUDE` ..now I'm wondering if `pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java` ln 157 `pluginsToLoad = Arrays.asList(pluginsInclude.split(","));` in master was even working in the first place..
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (04ec583) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `0.27%`.
> The diff coverage is `51.85%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
============================================
- Coverage 71.65% 71.38% -0.28%
- Complexity 4080 4082 +2
============================================
Files 1581 1583 +2
Lines 81350 81885 +535
Branches 12128 12242 +114
============================================
+ Hits 58293 58452 +159
- Misses 19117 19475 +358
- Partials 3940 3958 +18
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `29.16% <12.96%> (-0.15%)` | :arrow_down: |
| integration2 | `27.70% <12.96%> (-0.11%)` | :arrow_down: |
| unittests1 | `68.48% <51.85%> (-0.33%)` | :arrow_down: |
| unittests2 | `14.41% <0.00%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `55.41% <44.68%> (+6.86%)` | :arrow_up: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `85.91% <100.00%> (+0.62%)` | :arrow_up: |
| [...m/function/JsonExtractScalarTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSnNvbkV4dHJhY3RTY2FsYXJUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `22.49% <0.00%> (-27.04%)` | :arrow_down: |
| [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `50.00% <0.00%> (-25.00%)` | :arrow_down: |
| [...ransform/function/IdentifierTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSWRlbnRpZmllclRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `69.76% <0.00%> (-8.50%)` | :arrow_down: |
| [...nction/DistinctCountBitmapAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50Qml0bWFwQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `49.74% <0.00%> (-7.78%)` | :arrow_down: |
| [...a/org/apache/pinot/core/common/DataBlockCache.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vRGF0YUJsb2NrQ2FjaGUuamF2YQ==) | `91.44% <0.00%> (-4.77%)` | :arrow_down: |
| [...r/helix/SegmentOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50T25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `58.49% <0.00%> (-4.72%)` | :arrow_down: |
| [...java/org/apache/pinot/core/common/DataFetcher.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vRGF0YUZldGNoZXIuamF2YQ==) | `85.71% <0.00%> (-4.25%)` | :arrow_down: |
| [...t/local/upsert/PartitionUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvUGFydGl0aW9uVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `76.23% <0.00%> (-3.98%)` | :arrow_down: |
| ... and [21 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...04ec583](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] xiangfu0 merged pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
xiangfu0 merged pull request #7871:
URL: https://github.com/apache/pinot/pull/7871
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r766875593
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
friendly ping, any ideas?
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (541911a) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `57.20%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
=============================================
- Coverage 71.65% 14.44% -57.21%
+ Complexity 4080 80 -4000
=============================================
Files 1581 1538 -43
Lines 81350 80002 -1348
Branches 12128 12037 -91
=============================================
- Hits 58293 11560 -46733
- Misses 19117 67586 +48469
+ Partials 3940 856 -3084
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.44% <0.00%> (-0.07%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-48.56%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1255 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...541911a](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765378990
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
This is a bit odd, the master bash code in `pinot-tools/src/main/resources/appAssemblerScriptTemplate` is using `export IFS=";"` when looping through `$PLUGINS_INCLUDE` ..now I'm wondering if `pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java` ln 157 `pluginsToLoad = Arrays.asList(pluginsInclude.split(","));` in master was even working in the first place..
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (04ec583) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `6.58%`.
> The diff coverage is `51.85%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
============================================
- Coverage 71.65% 65.07% -6.59%
- Complexity 4080 4082 +2
============================================
Files 1581 1538 -43
Lines 81350 80015 -1335
Branches 12128 12039 -89
============================================
- Hits 58293 52071 -6222
- Misses 19117 24228 +5111
+ Partials 3940 3716 -224
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `68.48% <51.85%> (-0.33%)` | :arrow_down: |
| unittests2 | `14.41% <0.00%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `55.41% <44.68%> (+6.86%)` | :arrow_up: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `85.91% <100.00%> (+0.62%)` | :arrow_up: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...he/pinot/common/messages/SegmentReloadMessage.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvU2VnbWVudFJlbG9hZE1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/core/data/manager/realtime/TimerService.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvVGltZXJTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...not/common/exception/HttpErrorStatusException.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL0h0dHBFcnJvclN0YXR1c0V4Y2VwdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [364 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...04ec583](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] priyen commented on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-988047521
@Jackie-Jiang @jadami10 Would you review this when you get a chance? Thanks!
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (04ec583) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `57.24%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
=============================================
- Coverage 71.65% 14.41% -57.25%
+ Complexity 4080 80 -4000
=============================================
Files 1581 1538 -43
Lines 81350 80015 -1335
Branches 12128 12039 -89
=============================================
- Hits 58293 11532 -46761
- Misses 19117 67637 +48520
+ Partials 3940 846 -3094
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.41% <0.00%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-48.56%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1255 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...04ec583](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] jadami10 commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
jadami10 commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765354043
##########
File path: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java
##########
@@ -390,27 +391,41 @@ protected void addMapperJarToDistributedCache(Job job, PinotFS outputDirFS, URI
}
protected void packPluginsToDistributedCache(Job job, PinotFS outputDirFS, URI stagingDirURI) {
- File pluginsRootDir = new File(PluginManager.get().getPluginsRootDir());
- if (pluginsRootDir.exists()) {
- try {
- File pluginsTarGzFile = File.createTempFile("pinot-plugins-", ".tar.gz");
- TarGzCompressionUtils.createTarGzFile(pluginsRootDir, pluginsTarGzFile);
-
- // Copy to staging directory
- Path cachedPluginsTarball = new Path(stagingDirURI.toString(), SegmentGenerationUtils.PINOT_PLUGINS_TAR_GZ);
- outputDirFS.copyFromLocalFile(pluginsTarGzFile, cachedPluginsTarball.toUri());
- job.addCacheFile(cachedPluginsTarball.toUri());
- } catch (Exception e) {
- LOGGER.error("Failed to tar plugins directory and upload to staging dir", e);
- throw new RuntimeException(e);
- }
+ String[] pluginDirectories = PluginManager.get().getPluginsDirectories();
+ if (pluginDirectories == null) {
+ LOGGER.warn("Plugin directories is null, nothing to pack to distributed cache");
+ return;
+ }
- String pluginsIncludes = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
- if (pluginsIncludes != null) {
- job.getConfiguration().set(PLUGINS_INCLUDE_PROPERTY_NAME, pluginsIncludes);
+ ArrayList<File> validPluginDirectories = new ArrayList();
+
+ for (String pluginsDirPath : pluginDirectories) {
+ File pluginsDir = new File(pluginsDirPath);
+ if (pluginsDir.exists()) {
+ validPluginDirectories.add(pluginsDir);
+ } else {
+ LOGGER.warn("Cannot find Pinot plugins directory at [{}]", pluginsDirPath);
+ return;
}
- } else {
Review comment:
ah good call. it was too misaligned for me to tell
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
+1 to priyen. plugins themselves use `;`. and most PATH-like things use `;` as well?
##########
File path: pinot-spi/src/test/java/org/apache/pinot/spi/plugin/PluginManagerTest.java
##########
@@ -42,22 +46,86 @@
private String _jarFile;
private File _jarDirFile;
- @BeforeClass
- public void setup() {
+ private File _p1;
+ private File _p1Copy;
+ private File _p2;
+ private File _p3;
+ private File _p4;
+ @BeforeClass
+ public void setup()
+ throws IOException {
_tempDir = new File(System.getProperty("java.io.tmpdir"), "pinot-plugin-test");
- _tempDir.delete();
+ FileUtils.deleteDirectory(_tempDir);
_tempDir.mkdirs();
+ }
- String jarDir = _tempDir + "/test-record-reader";
- _jarFile = jarDir + "/test-record-reader.jar";
- _jarDirFile = new File(jarDir);
- _jarDirFile.mkdirs();
+ @Test
+ public void testGetPluginsToLoad()
Review comment:
solid test!
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765202664
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,55 +124,66 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
- }
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
- }
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
+ String[] directories = pluginsDirectories.split(";");
+ LOGGER.info("Plugin directories env: {}, parsed directories to load: '{}'", pluginsDirectories, directories);
+
+ for (String pluginsDirectory : directories) {
+ if (!new File(pluginsDirectory).exists()) {
+ LOGGER.warn("Plugins dir [{}] doesn't exist.", pluginsDirectory);
+ return;
+ }
+
+ Collection<File> jarFiles = FileUtils.listFiles(
+ new File(pluginsDirectory),
+ new String[]{JAR_FILE_EXTENSION},
+ true
+ );
+ List<String> pluginsToLoad = null;
+ if (!StringUtils.isEmpty(pluginsInclude)) {
+ pluginsToLoad = Arrays.asList(pluginsInclude.split(";"));
Review comment:
That's a good point, adding a check to see if it's already loaded and skipping subsequent ones if we find the same plugin
##########
File path: pinot-tools/src/main/resources/appAssemblerScriptTemplate
##########
@@ -109,35 +109,41 @@ fi
# Set $PLUGINS_CLASSPATH for plugin jars to be put into classpath.
# $PLUGINS_DIR and $PLUGINS_INCLUDE are used if $PLUGINS_CLASSPATH is not set.
-# $PLUGINS_DIR is the root directory of plugins directory, default to '"$BASEDIR"/plugins' if not set.
+# $PLUGINS_DIR is semi-colon separated list of plugin directories, default to '"$BASEDIR"/plugins' if not set.
# $PLUGINS_INCLUDE is semi-colon separated plugins name, e.g. pinot-avro;pinot-batch-ingestion-standalone. Default is not set, which means load all the plugin jars.
if [ -z "$PLUGINS_CLASSPATH" ] ; then
if [ -z "$PLUGINS_DIR" ] ; then
PLUGINS_DIR="$BASEDIR"/plugins
fi
- if [ -d "$PLUGINS_DIR" ] ; then
- if [ -n "$PLUGINS_INCLUDE" ] ; then
- export IFS=";"
- for PLUGIN_JAR in $PLUGINS_INCLUDE; do
- PLUGIN_JAR_PATH=$(find "$PLUGINS_DIR" -path \*/"$PLUGIN_JAR"/"$PLUGIN_JAR"-\*.jar)
+
+ export IFS=';'
+ for DIR in $PLUGINS_DIR; do
+ if [ -d "$DIR" ] ; then
+ unset IFS
+ if [ -n "$PLUGINS_INCLUDE" ] ; then
+ export IFS=';'
+ for PLUGIN_JAR in $PLUGINS_INCLUDE; do
+ PLUGIN_JAR_PATH=$(find "$DIR" -path \*/"$PLUGIN_JAR"/"$PLUGIN_JAR"-\*.jar)
+ if [ -n "$PLUGINS_CLASSPATH" ] ; then
+ PLUGINS_CLASSPATH=$PLUGINS_CLASSPATH:$PLUGIN_JAR_PATH
Review comment:
Good call, i'll add a comment to clarify this
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765202664
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,55 +124,66 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
- }
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
- }
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
+ String[] directories = pluginsDirectories.split(";");
+ LOGGER.info("Plugin directories env: {}, parsed directories to load: '{}'", pluginsDirectories, directories);
+
+ for (String pluginsDirectory : directories) {
+ if (!new File(pluginsDirectory).exists()) {
+ LOGGER.warn("Plugins dir [{}] doesn't exist.", pluginsDirectory);
+ return;
+ }
+
+ Collection<File> jarFiles = FileUtils.listFiles(
+ new File(pluginsDirectory),
+ new String[]{JAR_FILE_EXTENSION},
+ true
+ );
+ List<String> pluginsToLoad = null;
+ if (!StringUtils.isEmpty(pluginsInclude)) {
+ pluginsToLoad = Arrays.asList(pluginsInclude.split(";"));
Review comment:
That's a good point, i'll update to a check to see if it's already loaded and skipping subsequent ones if we find the same plugin
--
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
[GitHub] [pinot] priyen commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
priyen commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765347562
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
I used `;` as the plugin to include property also used it to be consistent, but I am fine with changing it to `,`, what do you think?
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (04ec583) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `1.09%`.
> The diff coverage is `51.85%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
============================================
- Coverage 71.65% 70.55% -1.10%
- Complexity 4080 4082 +2
============================================
Files 1581 1583 +2
Lines 81350 81885 +535
Branches 12128 12242 +114
============================================
- Hits 58293 57776 -517
- Misses 19117 20143 +1026
- Partials 3940 3966 +26
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `29.16% <12.96%> (-0.15%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `68.48% <51.85%> (-0.33%)` | :arrow_down: |
| unittests2 | `14.41% <0.00%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `55.41% <44.68%> (+6.86%)` | :arrow_up: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `85.91% <100.00%> (+0.62%)` | :arrow_up: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ager/realtime/PeerSchemeSplitSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGVlclNjaGVtZVNwbGl0U2VnbWVudENvbW1pdHRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...he/pinot/core/plan/StreamingSelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...ator/streaming/StreamingSelectionOnlyOperator.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-87.81%)` | :arrow_down: |
| [...ller/api/access/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvYWNjZXNzL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/core/auth/BasicAuthUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hdXRoL0Jhc2ljQXV0aFV0aWxzLmphdmE=) | `0.00% <0.00%> (-77.28%)` | :arrow_down: |
| [...he/pinot/common/utils/grpc/GrpcRequestBuilder.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUmVxdWVzdEJ1aWxkZXIuamF2YQ==) | `0.00% <0.00%> (-72.73%)` | :arrow_down: |
| ... and [88 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...04ec583](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#discussion_r765335287
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java
##########
@@ -56,15 +56,27 @@ private TarGzCompressionUtils() {
*/
public static void createTarGzFile(File inputFile, File outputFile)
throws IOException {
+ createTarGzFile(new File[] {inputFile}, outputFile);
Review comment:
(formatting) Please apply the pinot style and reformat the code: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java
##########
@@ -124,58 +125,104 @@ public synchronized void init() {
return;
}
try {
- _pluginsRootDir = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
+ _pluginsDirectories = System.getProperty(PLUGINS_DIR_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_DIR_PROPERTY_NAME, e);
- _pluginsRootDir = null;
+ _pluginsDirectories = null;
}
try {
_pluginsInclude = System.getProperty(PLUGINS_INCLUDE_PROPERTY_NAME);
} catch (Exception e) {
LOGGER.error("Failed to load env variable {}", PLUGINS_INCLUDE_PROPERTY_NAME, e);
_pluginsInclude = null;
}
- init(_pluginsRootDir, _pluginsInclude);
+ init(_pluginsDirectories, _pluginsInclude);
_initialized = true;
}
- private void init(String pluginsRootDir, String pluginsInclude) {
- if (StringUtils.isEmpty(pluginsRootDir)) {
+ private void init(String pluginsDirectories, String pluginsInclude) {
+ if (StringUtils.isEmpty(pluginsDirectories)) {
LOGGER.info("Env variable '{}' is not specified. Set this env variable to load additional plugins.",
PLUGINS_DIR_PROPERTY_NAME);
return;
} else {
- if (!new File(pluginsRootDir).exists()) {
- LOGGER.warn("Plugins root dir [{}] doesn't exist.", pluginsRootDir);
- return;
+ try {
+ HashMap<String, File> plugins = getPluginsToLoad(pluginsDirectories, pluginsInclude);
+ LOGGER.info("#getPluginsToLoad has produced {} plugins to load", plugins.size());
+
+ for (Map.Entry<String, File> entry : plugins.entrySet()) {
+ String pluginName = entry.getKey();
+ File pluginDir = entry.getValue();
+
+ try {
+ load(pluginName, pluginDir);
+ LOGGER.info("Successfully Loaded plugin [{}] from dir [{}]", pluginName, pluginDir);
+ } catch (Exception e) {
+ LOGGER.error("Failed to load plugin [{}] from dir [{}]", pluginName, pluginDir, e);
+ }
+ }
+
+ initRecordReaderClassMap();
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn(e.getMessage());
}
- LOGGER.info("Plugins root dir is [{}]", pluginsRootDir);
- }
- Collection<File> jarFiles = FileUtils.listFiles(new File(pluginsRootDir), new String[]{JAR_FILE_EXTENSION}, true);
- List<String> pluginsToLoad = null;
- if (!StringUtils.isEmpty(pluginsInclude)) {
- pluginsToLoad = Arrays.asList(pluginsInclude.split(","));
- LOGGER.info("Trying to load plugins: [{}]", Arrays.toString(pluginsToLoad.toArray()));
- } else {
- LOGGER.info("Please use env variable '{}' to customize plugins to load. Loading all plugins: {}",
- PLUGINS_INCLUDE_PROPERTY_NAME, Arrays.toString(jarFiles.toArray()));
}
- for (File jarFile : jarFiles) {
- File pluginDir = jarFile.getParentFile();
- String pluginName = pluginDir.getName();
- if (pluginsToLoad != null) {
- if (!pluginsToLoad.contains(pluginName)) {
- continue;
- }
+ }
+
+ /**
+ * This method will take a semi-colon delimited string of directories and a semi-colon delimited string of plugin
+ * names. It will traverse the directories in order and produce a <String, File> map of plugins to be loaded.
+ * If a plugin is found in multiple directories, only the first copy of it will be picked up.
+ * @param pluginsDirectories
+ * @param pluginsInclude
+ * @return A hash map with key = plugin name, value = file object
+ */
+ @VisibleForTesting
+ public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+ IllegalArgumentException {
+ String[] directories = pluginsDirectories.split(";");
Review comment:
I feel it is more common to use `,` as the array separator (e.g. in Apache commons configuration). Is there some special reason why picking `;` as the separator here?
(minor) Use `StringUtils.split(pluginsDirectories, ',')` for slightly better performance (avoid regex checking)
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7871: Support loading plugins from multiple directories
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7871:
URL: https://github.com/apache/pinot/pull/7871#issuecomment-987260773
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7871](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (166d330) into [master](https://codecov.io/gh/apache/pinot/commit/e572ea02dd942e3eeaad71233b91d8d003f5f2d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e572ea0) will **decrease** coverage by `43.74%`.
> The diff coverage is `18.42%`.
[](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7871 +/- ##
=============================================
- Coverage 71.65% 27.91% -43.75%
=============================================
Files 1581 1572 -9
Lines 81350 81015 -335
Branches 12128 12093 -35
=============================================
- Hits 58293 22613 -35680
- Misses 19117 56346 +37229
+ Partials 3940 2056 -1884
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `27.91% <18.42%> (+0.10%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/spi/plugin/PluginManager.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbk1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-48.56%)` | :arrow_down: |
| [...ache/pinot/common/utils/TarGzCompressionUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGFyR3pDb21wcmVzc2lvblV0aWxzLmphdmE=) | `63.38% <100.00%> (-21.92%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/readers/FileFormat.java](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9yZWFkZXJzL0ZpbGVGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1129 more](https://codecov.io/gh/apache/pinot/pull/7871/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e572ea0...166d330](https://codecov.io/gh/apache/pinot/pull/7871?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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