[GitHub] [pinot] xiangfu0 commented on a change in pull request #7871: Support loading plugins from multiple directories

[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