[GitHub] [hadoop] szetszwo commented on a change in pull request #2172: HDFS-15483. Ordered snapshot deletion: Disallow rename between two snapshottable directories.

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

szetszwo commented on a change in pull request #2172:
URL: https://github.com/apache/hadoop/pull/2172#discussion_r470189729



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
##########
@@ -358,4 +358,23 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip,
       checkSnapshot(iip.getLastINode(), snapshottableDirs);
     }
   }
+
+  static void checkUnderSameSnapshottableRoot(FSDirectory fsd,
+      INodesInPath srcIIP, INodesInPath dstIIP) throws IOException {
+    // Ensure rename out of a snapshottable root is not permitted if ordered
+    // snapshot deletion feature is enabled
+    SnapshotManager snapshotManager = fsd.getFSNamesystem().
+        getSnapshotManager();
+    if (snapshotManager.isSnapshotDeletionOrdered() && fsd.getFSNamesystem()
+        .isSnapshotTrashRootEnabled()) {
+      String errMsg = "Source " + srcIIP.getPath() +
+          " and dest " + dstIIP.getPath() + " are not under " +
+          "the same snapshot root.";
+      INodeDirectory src = snapshotManager.

Review comment:
       src can be null.  We should add a test for this case.

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
##########
@@ -358,4 +358,23 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip,
       checkSnapshot(iip.getLastINode(), snapshottableDirs);
     }
   }
+
+  static void checkUnderSameSnapshottableRoot(FSDirectory fsd,
+      INodesInPath srcIIP, INodesInPath dstIIP) throws IOException {
+    // Ensure rename out of a snapshottable root is not permitted if ordered
+    // snapshot deletion feature is enabled
+    SnapshotManager snapshotManager = fsd.getFSNamesystem().
+        getSnapshotManager();
+    if (snapshotManager.isSnapshotDeletionOrdered() && fsd.getFSNamesystem()
+        .isSnapshotTrashRootEnabled()) {
+      String errMsg = "Source " + srcIIP.getPath() +
+          " and dest " + dstIIP.getPath() + " are not under " +
+          "the same snapshot root.";

Review comment:
       The errMsg should be constructed right before "throw new SnapshotException(errMsg);"

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
##########
@@ -363,23 +363,38 @@ void assertFirstSnapshot(INodeDirectory dir,
    * @param iip INodesInPath for the directory to get snapshot root.
    * @return the snapshot root INodeDirectory
    */
+  public INodeDirectory checkAndGetSnapshottableAncestorDir(
+      final INodesInPath iip) throws IOException {
+    final INodeDirectory dir = getSnapshottableAncestorDir(iip);
+    if (dir == null) {
+      throw new SnapshotException("Directory is neither snapshottable nor" +
+          " under a snap root!");
+    }
+    return dir;
+  }
+
   public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip)
       throws IOException {
     final String path = iip.getPath();
-    final INodeDirectory dir = INodeDirectory.valueOf(iip.getLastINode(), path);
-    if (dir.isSnapshottable()) {
-      return dir;
+    final INode inode = iip.getLastINode();
+    final INodeDirectory dir;
+    if (inode instanceof INodeDirectory) {
+      dir = INodeDirectory.valueOf(inode, path);
+      if (dir.isSnapshottable()) {
+        return dir;
+      }

Review comment:
       This check should be moved to right before the for-loop below (line 389).

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
##########
@@ -358,4 +358,23 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip,
       checkSnapshot(iip.getLastINode(), snapshottableDirs);
     }
   }
+
+  static void checkUnderSameSnapshottableRoot(FSDirectory fsd,

Review comment:
       This is only used in FSDirRenameOp.  Let's move to there?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org