Allow rename() to overwrite, fix an uncaught ex and reenable UT

### What changes are proposed in this pull request? As the title states, this PR does a few things: 1. Allow rename() to overwrite an existing path, if specified in the option 2. The method `rename` in `AbstractFileSystem`, now will process the input path and rerun `rename` method when caught `AlluxioException` or `AlluxioRuntimeException`. Instead of log the exception and return end. 3. Also add some path checks during the rename 4. Reenable `FileSystemRenameIntegrationTest` by adding configs to `LocalAlluxioClusterResource` and reenable UT cases. pr-link: #18263 change-id: cid-2870bf87fea8a3b2419e5b10a05423ff2dede6a2
Alluxio · Nov 10, 2023 · 328ee8c · 328ee8c
1 parent 5421aa4
commit 328ee8c
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 42 deletions.
diff --git a/common/transport/src/main/proto/grpc/file_system_master.proto b/common/transport/src/main/proto/grpc/file_system_master.proto
@@ -463,6 +463,7 @@ message RenamePOptions {
   optional FileSystemMasterCommonPOptions commonOptions = 1;
   optional bool persist = 2;
   optional S3SyntaxOptions s3SyntaxOptions = 3;
+  optional bool overwrite = 4;
 }
 message RenamePRequest {
   /** the source path of the file or directory */

diff --git a/dora/core/client/fs/src/main/java/alluxio/client/file/DoraCacheFileSystem.java b/dora/core/client/fs/src/main/java/alluxio/client/file/DoraCacheFileSystem.java
@@ -30,6 +30,7 @@
 import alluxio.exception.InvalidPathException;
 import alluxio.exception.OpenDirectoryException;
 import alluxio.exception.runtime.AlluxioRuntimeException;
+import alluxio.exception.status.FailedPreconditionException;
 import alluxio.grpc.CreateDirectoryPOptions;
 import alluxio.grpc.CreateFilePOptions;
 import alluxio.grpc.DeletePOptions;
@@ -64,6 +65,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -383,6 +385,19 @@ public void rename(AlluxioURI src, AlluxioURI dst, RenamePOptions options)
 
       mDoraClient.rename(srcUfsFullPath.toString(), dstUfsFullPath.toString(), mergedOptions);
     } catch (RuntimeException ex) {
+      if (ex instanceof StatusRuntimeException) {
+        Status.Code code = ((StatusRuntimeException) ex).getStatus().getCode();
+        if (Status.FAILED_PRECONDITION.getCode().equals(code)) {
+          throw new FailedPreconditionException(String.format(
+              "Precondition failed: cannot rename %s to %s", src.toString(), dst.toString()));
+        } else if (Status.NOT_FOUND.getCode().equals(code)) {
+          throw new FileNotFoundException(ex.getMessage());
+        } else if (Status.ALREADY_EXISTS.getCode().equals(code)) {
+          // throw exception here, no fallback even the fallback is open
+          // which means even ufs support overwrite, alluxio won't allow doing it
+          throw new FileAlreadyExistsException(ex.getMessage());
+        }
+      }
       if (!mUfsFallbackEnabled) {
         throw ex;
       }

diff --git a/dora/core/client/hdfs/src/main/java/alluxio/hadoop/AbstractFileSystem.java b/dora/core/client/hdfs/src/main/java/alluxio/hadoop/AbstractFileSystem.java
@@ -735,7 +735,10 @@ public boolean rename(Path src, Path dst) throws IOException {
     if (mStatistics != null) {
       mStatistics.incrementWriteOps(1);
     }
-
+    if (src.equals(dst)) {
+      LOG.debug("Renaming {} to the same location", src);
+      return true;
+    }
     AlluxioURI srcPath = getAlluxioPath(src);
     AlluxioURI dstPath = getAlluxioPath(dst);
     try {
@@ -745,34 +748,53 @@ public boolean rename(Path src, Path dst) throws IOException {
       return false;
     } catch (InvalidArgumentRuntimeException e) {
       throw new IllegalArgumentException(e);
-    } catch (AlluxioRuntimeException e) {
-      throw toHdfsIOException(e);
-    } catch (AlluxioException e) {
-      ensureExists(srcPath);
-      URIStatus dstStatus;
-      try {
-        dstStatus = mFileSystem.getStatus(dstPath);
-      } catch (IOException | AlluxioException e2) {
-        LOG.warn("rename failed: {}", e.toString());
-        return false;
-      }
-      // If the destination is an existing folder, try to move the src into the folder
-      if (dstStatus != null && dstStatus.isFolder()) {
-        dstPath = dstPath.joinUnsafe(srcPath.getName());
-      } else {
-        LOG.warn("rename failed: {}", e.toString());
-        return false;
+    } catch (AlluxioRuntimeException | AlluxioException e) {
+      Exception exToLog = e;
+      if (e instanceof  AlluxioRuntimeException) {
+        exToLog = toHdfsIOException((AlluxioRuntimeException) e);
       }
       try {
-        mFileSystem.rename(srcPath, dstPath);
-      } catch (IOException | AlluxioException e2) {
-        LOG.error("Failed to rename {} to {}", src, dst, e2);
+        boolean res = tryMoveIntoDirectory(src, dst, srcPath, dstPath);
+        if (!res) {
+          LOG.warn("Failed to rename src {} to dst {}"
+              + "because the dst is not a directory", src,  dst, exToLog);
+          return false;
+        }
+      } catch (IOException | AlluxioException | AlluxioRuntimeException e2) {
+        LOG.warn("Failed to rename src {} to dst {} with exception {},"
+                + "and fail to move src to dst as a folder with exception",
+            src, dst, exToLog, e2);
         return false;
       }
-    } catch (IOException e) {
-      LOG.error("Failed to rename {} to {}", src, dst, e);
+    } catch (IOException | RuntimeException e) {
+      LOG.warn("Failed to rename {} to {}", src, dst, e);
+      return false;
+    }
+    return true;
+  }
+
+  /**
+   * rename when previous rename fail, reconstruct path and check status.
+   * In order to support hdfs rename interface
+   *
+   * @param src src
+   * @param dst dst
+   * @param srcPath srcPath
+   * @param dstPath srcPath
+   * @return true in success, false if fail
+   */
+  public boolean tryMoveIntoDirectory(Path src, Path dst, AlluxioURI srcPath, AlluxioURI dstPath)
+      throws IOException, AlluxioException {
+    ensureExists(srcPath);
+    URIStatus dstStatus;
+    dstStatus = mFileSystem.getStatus(dstPath);
+    // If the destination is an existing folder, try to move the src into the folder
+    if (dstStatus != null && dstStatus.isFolder()) {
+      dstPath = dstPath.joinUnsafe(srcPath.getName());
+    } else {
       return false;
     }
+    mFileSystem.rename(srcPath, dstPath);
     return true;
   }
 

diff --git a/dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java b/dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java
@@ -33,6 +33,7 @@
 import alluxio.conf.PropertyKey;
 import alluxio.exception.AccessControlException;
 import alluxio.exception.runtime.AlluxioRuntimeException;
+import alluxio.exception.runtime.AlreadyExistsRuntimeException;
 import alluxio.exception.runtime.FailedPreconditionRuntimeException;
 import alluxio.exception.runtime.UnavailableRuntimeException;
 import alluxio.exception.status.AlreadyExistsException;
@@ -1024,6 +1025,20 @@ public void rename(String src, String dst, RenamePOptions options)
       throw new FailedPreconditionException("Cannot rename a file in one UFS to another UFS");
     }
 
+    try {
+      // Check if the target file already exists. If yes, return by throwing error.
+      boolean overWrite = options.hasOverwrite() ? options.getOverwrite() : false;
+      boolean exists = srcUfs.exists(dst);
+      if (!overWrite && exists) {
+        throw new AlreadyExistsRuntimeException(String.format("File %s already exists but"
+            + "no overwrite flag", dst));
+      } else if (overWrite) {
+        mMetaManager.removeFromMetaStore(dst);
+      }
+    } catch (IOException e) {
+      throw new RuntimeException(e);
+    }
+
     boolean rc;
     try {
       UfsStatus status = srcUfs.getStatus(src);

diff --git a/...ests/integration/src/test/java/alluxio/client/hadoop/FileSystemRenameIntegrationTest.java b/...ests/integration/src/test/java/alluxio/client/hadoop/FileSystemRenameIntegrationTest.java
@@ -11,7 +11,6 @@
 
 package alluxio.client.hadoop;
 
-import alluxio.annotation.dora.DoraTestTodoItem;
 import alluxio.client.WriteType;
 import alluxio.conf.Configuration;
 import alluxio.conf.PropertyKey;
@@ -34,18 +33,17 @@
 import java.io.IOException;
 import java.net.URI;
 
-/**
- * Integration tests for {@link FileSystem#rename(Path, Path)}.
- */
-// TODO(jiri): Test persisting rename operations to UFS.
-@DoraTestTodoItem(action = DoraTestTodoItem.Action.REMOVE, owner = "jiaming",
-    comment = "adapt rename to the new arch")
-@Ignore
 public final class FileSystemRenameIntegrationTest extends BaseIntegrationTest {
   @ClassRule
   public static LocalAlluxioClusterResource sLocalAlluxioClusterResource =
       new LocalAlluxioClusterResource.Builder()
-          .setProperty(PropertyKey.USER_FILE_WRITE_TYPE_DEFAULT, WriteType.CACHE_THROUGH).build();
+          .setProperty(PropertyKey.USER_FILE_WRITE_TYPE_DEFAULT, WriteType.CACHE_THROUGH)
+          .setProperty(PropertyKey.UNDERFS_XATTR_CHANGE_ENABLED, false)
+          .setProperty(PropertyKey.USER_FILE_METADATA_SYNC_INTERVAL, 0)
+          .setProperty(PropertyKey.DORA_WORKER_METASTORE_ROCKSDB_TTL, 0)
+          .setProperty(PropertyKey.DORA_UFS_LIST_STATUS_CACHE_TTL, 0)
+          .setProperty(PropertyKey.DORA_CLIENT_UFS_FALLBACK_ENABLED, false)
+          .build();
   private static String sUfsRoot;
   private static UnderFileSystem sUfs;
   private static org.apache.hadoop.fs.FileSystem sTFS;
@@ -82,7 +80,6 @@ public static void beforeClass() throws Exception {
   }
 
   @Test
-  @Ignore
   public void basicRenameTest1() throws Exception {
     // Rename /fileA to /fileB
     Path fileA = new Path("/fileA");
@@ -104,7 +101,6 @@ public void basicRenameTest1() throws Exception {
   }
 
   @Test
-  @Ignore
   public void basicRenameTest2() throws Exception {
     // Rename /fileA to /dirA/fileA
     Path fileA = new Path("/fileA");
@@ -129,7 +125,6 @@ public void basicRenameTest2() throws Exception {
   }
 
   @Test
-  @Ignore
   public void basicRenameTest3() throws Exception {
     // Rename /fileA to /dirA/fileA without specifying the full path
     Path fileA = new Path("/fileA");
@@ -154,7 +149,6 @@ public void basicRenameTest3() throws Exception {
   }
 
   @Test
-  @Ignore
   public void basicRenameTest4() throws Exception {
     // Rename /fileA to /fileA
     Path fileA = new Path("/fileA");
@@ -173,7 +167,6 @@ public void basicRenameTest4() throws Exception {
   }
 
   @Test
-  @Ignore
   public void basicRenameTest5() throws Exception {
     // Rename /fileA to /fileAfileA
     Path fileA = new Path("/fileA");
@@ -195,7 +188,6 @@ public void basicRenameTest5() throws Exception {
   }
 
   @Test
-  @Ignore
   public void basicRenameTest6() throws Exception {
     // Rename /dirA to /dirB, /dirA/fileA should become /dirB/fileA
     Path dirA = new Path("/dirA");
@@ -262,7 +254,6 @@ public void basicRenameTest7() throws Exception {
   }
 
   @Test
-  @Ignore
   public void errorRenameTest1() throws Exception {
     // Rename /dirA to /dirA/dirB should fail
     Path dirA = new Path("/dirA");
@@ -284,7 +275,6 @@ public void errorRenameTest1() throws Exception {
   }
 
   @Test
-  @Ignore
   public void errorRenameTest2() throws Exception {
     // Rename /fileA to /fileB should fail if /fileB exists
     Path fileA = new Path("/fileA");
@@ -309,7 +299,6 @@ public void errorRenameTest2() throws Exception {
   }
 
   @Test
-  @Ignore
   public void errorRenameTest3() throws Exception {
     // Rename /fileA to /dirA/fileA should fail if /dirA/fileA exists
     Path fileA = new Path("/fileA");
@@ -339,7 +328,6 @@ public void errorRenameTest3() throws Exception {
   }
 
   @Test
-  @Ignore
   public void errorRenameTest4() throws Exception {
     // Rename /fileA to an nonexistent path should fail
     Path fileA = new Path("/fileA");