[GitHub] [hadoop] szetszwo commented on a change in pull request #2163: HDFS-15480. Ordered snapshot deletion: record snapshot deletion in XAttr

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

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



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -359,14 +359,6 @@ public int getListLimit() {
             DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED_DEFAULT);
     LOG.info("{} = {}", DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED,
         snapshotDeletionOrdered);
-    if (snapshotDeletionOrdered && !xattrsEnabled) {

Review comment:
       Why removing the check?

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
##########
@@ -263,11 +269,18 @@ static SnapshotDiffReportListing getSnapshotDiffReportListing(FSDirectory fsd,
       final int earliest = snapshottable.getDiffs().iterator().next()
           .getSnapshotId();
       if (snapshot.getId() != earliest) {
-        throw new SnapshotException("Failed to delete snapshot " + snapshotName
-            + " from directory " + srcRoot.getFullPathName()
-            + ": " + snapshot + " is not the earliest snapshot id=" + earliest
-            + " (" + DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED
-            + " is " + fsd.isSnapshotDeletionOrdered() + ")");
+        final XAttr snapshotXAttr = buildXAttr(snapshotName);
+        final List<XAttr> xattrs = Lists.newArrayListWithCapacity(1);
+        xattrs.add(snapshotXAttr);
+
+        // The snapshot to be deleted is just marked for deletion in the xAttr.
+        // Same snaphot delete call can happen multiple times until annd unless
+        // the very 1st instance of a snapshot delete hides it/remove it from
+        // snapshot list. XAttrSetFlag.REPLACE needs to be set to here in order
+        // to address this.
+        FSDirXAttrOp.unprotectedSetXAttrs(fsd, iip, xattrs,
+            EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE));
+        return null;

Review comment:
       We cannot return null since it will skip the fsd.getEditLog().logDeleteSnapshot(..).

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
##########
@@ -5119,7 +5119,14 @@
     for storing directory snapshot diffs. By default, value is set to 10.
   </description>
 </property>
-
+<property>
+  <name>dfs.namenode.snapshot.deletion.ordered</name>

Review comment:
       @arp7 seems to suggest not to add the conf in hdfs-default.xml.

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java
##########
@@ -326,6 +323,11 @@ static INode unprotectedSetXAttrs(
         throw new IOException("Can only set '" +
             SECURITY_XATTR_UNREADABLE_BY_SUPERUSER + "' on a file.");
       }
+
+      if (xaName.contains(SNAPSHOT_XATTR_NAME)) {
+        Preconditions.checkArgument(inode.isDirectory() &&

Review comment:
       To be consistent with SECURITY_XATTR_UNREADABLE_BY_SUPERUSER, should we throw IOException?
   




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