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

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

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



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
     }
 
     validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs);
-
+    FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP);

Review comment:
       SnapshotRename check should happen irrespective of orderedDeletion config is set or not. Plus snapshotRename will always within the snapshottable root implicitly. Any speciofic reason to merge these two checks at once?

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
     }
 
     validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs);
-
+    FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP);

Review comment:
       > There are 2 callers of fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
   FSDirSnapshotOp.checkUnderSameSnapshottableRoo() is also called from same two functions.

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
##########
@@ -341,4 +341,24 @@ 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
+    if (fsd.isSnapshotDeletionOrdered()) {
+      SnapshotManager snapshotManager = fsd.getFSNamesystem().
+          getSnapshotManager();
+      String errMsg = "Source " + srcIIP.getPath() +
+          " and dest " + dstIIP.getPath() + " are not under " +
+          "the same snapshot root.";
+      INodeDirectory src = snapshotManager.
+          getSnapshottableAncestorDir(srcIIP);
+      INodeDirectory dst = snapshotManager.getSnapshottableAncestorDir(dstIIP);
+      if (!(dstIIP.isDescendant(snapshotManager.
+          getSnapshottableAncestorDir(srcIIP)))) {

Review comment:
       done

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
     }
 
     validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs);
-
+    FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP);

Review comment:
       SnapshotRename check should happen irrespective of orderedDeletion config is set or not. Plus snapshotRename will always within the snapshottable root implicitly. Also, if just directories are marked snapshottable but snapshots exist, the rename will still fail across snapshottable roots. The two checks seem to be mutually exclusive to me.
   
   Any specific reason to merge these two checks at once?




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