From 743c2e9071f4a73e0196ad4ca005b767758642b9 Mon Sep 17 00:00:00 2001
From: Surendra Singh Lilhore <surendralilhore@apache.org>
Date: Wed, 13 May 2020 15:01:07 +0530
Subject: [PATCH] HDFS-15316. Deletion failure should not remove directory from
 snapshottables. Contributed by hemanthboyina

---
 .../hdfs/server/namenode/FSDirDeleteOp.java   |  4 +-
 .../hdfs/server/namenode/TestDeleteRace.java  | 46 +++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java
index 47f25f6f1fb..2dfb90ee672 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirDeleteOp.java
@@ -61,10 +61,10 @@ class FSDirDeleteOp {
             removedUCFiles);
         if (unprotectedDelete(fsd, iip, context, mtime)) {
           filesRemoved = context.quotaDelta().getNsDelta();
+          fsn.removeSnapshottableDirs(snapshottableDirs);
         }
         fsd.updateReplicationFactor(context.collectedBlocks()
                                         .toUpdateReplicationInfo());
-        fsn.removeSnapshottableDirs(snapshottableDirs);
         fsd.updateCount(iip, context.quotaDelta(), false);
       }
     } finally {
@@ -144,9 +144,9 @@ class FSDirDeleteOp {
         new ReclaimContext(fsd.getBlockStoragePolicySuite(),
             collectedBlocks, removedINodes, removedUCFiles),
         mtime);
-    fsn.removeSnapshottableDirs(snapshottableDirs);
 
     if (filesRemoved) {
+      fsn.removeSnapshottableDirs(snapshottableDirs);
       fsn.removeLeasesAndINodes(removedUCFiles, removedINodes, false);
       fsn.getBlockManager().removeBlocksAndUpdateSafemodeTotal(collectedBlocks);
     }
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java
index 923750cfe8c..17146dabfaa 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeleteRace.java
@@ -69,6 +69,8 @@ import org.mockito.Mockito;
 
 import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_LEASE_RECHECK_INTERVAL_MS_DEFAULT;
 import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_LEASE_RECHECK_INTERVAL_MS_KEY;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
 import static org.junit.Assert.assertNotEquals;
 
 /**
@@ -513,4 +515,48 @@ public class TestDeleteRace {
       }
     }
   }
+
+  /**
+   * Test get snapshot diff on a directory which delete got failed.
+   */
+  @Test
+  public void testDeleteOnSnapshottableDir() throws Exception {
+    conf.setBoolean(
+        DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DIFF_ALLOW_SNAP_ROOT_DESCENDANT,
+        true);
+    try {
+      cluster = new MiniDFSCluster.Builder(conf).numDataNodes(1).build();
+      cluster.waitActive();
+      DistributedFileSystem hdfs = cluster.getFileSystem();
+      FSNamesystem fsn = cluster.getNamesystem();
+      FSDirectory fsdir = fsn.getFSDirectory();
+      Path dir = new Path("/dir");
+      hdfs.mkdirs(dir);
+      hdfs.allowSnapshot(dir);
+      Path file1 = new Path(dir, "file1");
+      Path file2 = new Path(dir, "file2");
+
+      // directory should not get deleted
+      FSDirectory fsdir2 = Mockito.spy(fsdir);
+      cluster.getNamesystem().setFSDirectory(fsdir2);
+      Mockito.doReturn(-1L).when(fsdir2).removeLastINode(any());
+      hdfs.delete(dir, true);
+
+      // create files and create snapshots
+      DFSTestUtil.createFile(hdfs, file1, BLOCK_SIZE, (short) 1, 0);
+      hdfs.createSnapshot(dir, "s1");
+      DFSTestUtil.createFile(hdfs, file2, BLOCK_SIZE, (short) 1, 0);
+      hdfs.createSnapshot(dir, "s2");
+
+      // should able to get snapshot diff on ancestor dir
+      Path dirDir1 = new Path(dir, "dir1");
+      hdfs.mkdirs(dirDir1);
+      hdfs.getSnapshotDiffReport(dirDir1, "s2", "s1");
+      assertEquals(1, fsn.getSnapshotManager().getNumSnapshottableDirs());
+    } finally {
+      if (cluster != null) {
+        cluster.shutdown();
+      }
+    }
+  }
 }