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

[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