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

[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