[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8200: add copy recursive API to pinotFS

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #8200:
URL: https://github.com/apache/pinot/pull/8200#discussion_r810292184



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java
##########
@@ -88,10 +88,25 @@ boolean move(URI srcUri, URI dstUri, boolean overwrite)
       throws IOException;
 
   /**
-   * Does the actual behavior of move in each FS.
+   * Copies the file from the src to dst. The original file is retained. If the dst has parent directories
+   * that haven't been created, this method will create all the necessary parent directories. If dst already exists,
+   * this will overwrite the existing file in the path.
+   *
+   * Note: In Pinot we recommend the full paths of both src and dst be specified.
+   * For example, if a file /a/b/c is copied to a file /x/y/z, the directory /a/b still exists containing the file 'c'.
+   * The dst file /x/y/z will contain the contents of 'c'.
+   * @param srcUri URI of the original file
+   * @param dstUri URI of the final file location
+   * @return true if copy is successful
+   * @throws IOException on IO failure
    */
-  boolean doMove(URI srcUri, URI dstUri)
-      throws IOException;
+  default boolean copy(URI srcUri, URI dstUri)
+      throws IOException {
+    if (isDirectory(srcUri)) {
+      throw new UnsupportedOperationException("Recursive copy not supported");

Review comment:
       ```suggestion
         throw new IllegalArgumentException("Cannot copy directory: " + srcUri);
   ```

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java
##########
@@ -108,8 +123,10 @@ boolean doMove(URI srcUri, URI dstUri)
    * @return true if copy is successful
    * @throws IOException on IO failure
    */
-  boolean copy(URI srcUri, URI dstUri)
-      throws IOException;
+  default boolean copyDir(URI srcUri, URI dstUri)

Review comment:
       Let's not adding default for `copyDir()`. All the implementation should have it implemented

##########
File path: pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzurePinotFS.java
##########
@@ -107,8 +107,12 @@ public boolean doMove(URI srcUri, URI dstUri)
   }
 
   @Override
-  public boolean copy(URI srcUri, URI dstUri)
+  public boolean copyDir(URI srcUri, URI dstUri)
       throws IOException {
+    if (isDirectory(srcUri)) {

Review comment:
       Is this true? If so, add a TODO to support recursive copy?

##########
File path: pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzurePinotFS.java
##########
@@ -126,9 +130,8 @@ public boolean copy(URI srcUri, URI dstUri)
       inputStream.close();
       outputStream.close();
     } catch (IOException e) {
-      LOGGER
-          .error("Exception encountered during copy, input: '{}', output: '{}'.", srcUri.toString(), dstUri.toString(),
-              e);
+      LOGGER.error("Exception encountered during copy, input: '{}', output: '{}'.", srcUri, dstUri, e);
+      return false;

Review comment:
       +1

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java
##########
@@ -88,10 +88,25 @@ boolean move(URI srcUri, URI dstUri, boolean overwrite)
       throws IOException;
 
   /**
-   * Does the actual behavior of move in each FS.
+   * Copies the file from the src to dst. The original file is retained. If the dst has parent directories

Review comment:
       Please revise the javadoc. Most of them only apply to directory

##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/PeerDownloadLLCRealtimeClusterIntegrationTest.java
##########
@@ -291,7 +291,7 @@ public boolean doMove(URI srcUri, URI dstUri)
     public boolean copy(URI srcUri, URI dstUri)

Review comment:
       This should override `copyDir`




-- 
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