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

[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