diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 802f59cedcd..82a649cb92e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -265,7 +265,7 @@ Trunk (Unreleased) HDFS-4687. TestDelegationTokenForProxyUser#testWebHdfsDoAs is flaky with JDK7. (Andrew Wang via atm) - BREAKDOWN OF HDFS-2802 HDFS SNAPSHOT SUBTASKS + BREAKDOWN OF HDFS-2802 HDFS SNAPSHOT SUBTASKS AND RELATED JIRAS HDFS-4076. Support snapshot of single files. (szetszwo) @@ -604,6 +604,9 @@ Trunk (Unreleased) HDFS-4809. When a QuotaExceededException is thrown during rename, the quota usage should be subtracted back. (Jing Zhao via szetszwo) + HDFS-4842. Identify the correct prior snapshot when deleting a + snapshot under a renamed subtree. (jing9) + Release 2.0.5-beta - UNRELEASED INCOMPATIBLE CHANGES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java index 101a158fbc1..3e9c71e2976 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java @@ -493,6 +493,11 @@ public abstract class INodeReference extends INode { if (prior == null) { prior = getPriorSnapshot(this); } + + if (prior != null + && Snapshot.ID_COMPARATOR.compare(snapshot, prior) <= 0) { + return Quota.Counts.newInstance(); + } Quota.Counts counts = getReferredINode().cleanSubtree(snapshot, prior, collectedBlocks, removedINodes); @@ -596,7 +601,11 @@ public abstract class INodeReference extends INode { if (prior == null) { prior = getPriorSnapshot(this); } - if (snapshot != null && snapshot.equals(prior)) { + // if prior is not null, and prior is not before the to-be-deleted + // snapshot, we can quit here and leave the snapshot deletion work to + // the src tree of rename + if (snapshot != null && prior != null + && Snapshot.ID_COMPARATOR.compare(snapshot, prior) <= 0) { return Quota.Counts.newInstance(); } return getReferredINode().cleanSubtree(snapshot, prior, diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java index 2f11c690d29..98f75bd12f5 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java @@ -1915,4 +1915,166 @@ public class TestRenameWithSnapshots { INodeFile barNode = (INodeFile) fsdir.getINode4Write(bar3.toString()); assertSame(fsdir.getINode4Write(dir3.toString()), barNode.getParent()); } + + /** + * Rename and deletion snapshot under the same the snapshottable directory. + */ + @Test + public void testRenameDirAndDeleteSnapshot_6() throws Exception { + final Path test = new Path("/test"); + final Path dir1 = new Path(test, "dir1"); + final Path dir2 = new Path(test, "dir2"); + hdfs.mkdirs(dir1); + hdfs.mkdirs(dir2); + + final Path foo = new Path(dir2, "foo"); + final Path bar = new Path(foo, "bar"); + final Path file = new Path(bar, "file"); + DFSTestUtil.createFile(hdfs, file, BLOCKSIZE, REPL, SEED); + + // take a snapshot on /test + SnapshotTestHelper.createSnapshot(hdfs, test, "s0"); + + // delete /test/dir2/foo/bar/file after snapshot s0, so that there is a + // snapshot copy recorded in bar + hdfs.delete(file, true); + + // rename foo from dir2 to dir1 + final Path newfoo = new Path(dir1, foo.getName()); + hdfs.rename(foo, newfoo); + + final Path foo_s0 = SnapshotTestHelper.getSnapshotPath(test, "s0", + "dir2/foo"); + assertTrue("the snapshot path " + foo_s0 + " should exist", + hdfs.exists(foo_s0)); + + // delete snapshot s0. The deletion will first go down through dir1, and + // find foo in the created list of dir1. Then it will use null as the prior + // snapshot and continue the snapshot deletion process in the subtree of + // foo. We need to make sure the snapshot s0 can be deleted cleanly in the + // foo subtree. + hdfs.deleteSnapshot(test, "s0"); + // check the internal + assertFalse("after deleting s0, " + foo_s0 + " should not exist", + hdfs.exists(foo_s0)); + INodeDirectoryWithSnapshot dir2Node = (INodeDirectoryWithSnapshot) fsdir + .getINode4Write(dir2.toString()); + assertTrue("the diff list of " + dir2 + + " should be empty after deleting s0", dir2Node.getDiffs().asList() + .isEmpty()); + + assertTrue(hdfs.exists(newfoo)); + INode fooRefNode = fsdir.getINode4Write(newfoo.toString()); + assertTrue(fooRefNode instanceof INodeReference.DstReference); + INodeDirectory fooNode = fooRefNode.asDirectory(); + // fooNode should be still INodeDirectoryWithSnapshot since we call + // recordModification before the rename + assertTrue(fooNode instanceof INodeDirectoryWithSnapshot); + assertTrue(((INodeDirectoryWithSnapshot) fooNode).getDiffs().asList() + .isEmpty()); + INodeDirectory barNode = fooNode.getChildrenList(null).get(0).asDirectory(); + // bar should also be an INodeDirectoryWithSnapshot, and both of its diff + // list and children list are empty + assertTrue(((INodeDirectoryWithSnapshot) barNode).getDiffs().asList() + .isEmpty()); + assertTrue(barNode.getChildrenList(null).isEmpty()); + + restartClusterAndCheckImage(); + } + + /** + * Unit test for HDFS-4842. + */ + @Test + public void testRenameDirAndDeleteSnapshot_7() throws Exception { + fsn.getSnapshotManager().setAllowNestedSnapshots(true); + final Path test = new Path("/test"); + final Path dir1 = new Path(test, "dir1"); + final Path dir2 = new Path(test, "dir2"); + hdfs.mkdirs(dir1); + hdfs.mkdirs(dir2); + + final Path foo = new Path(dir2, "foo"); + final Path bar = new Path(foo, "bar"); + final Path file = new Path(bar, "file"); + DFSTestUtil.createFile(hdfs, file, BLOCKSIZE, REPL, SEED); + + // take a snapshot s0 and s1 on /test + SnapshotTestHelper.createSnapshot(hdfs, test, "s0"); + SnapshotTestHelper.createSnapshot(hdfs, test, "s1"); + // delete file so we have a snapshot copy for s1 in bar + hdfs.delete(file, true); + + // create another snapshot on dir2 + SnapshotTestHelper.createSnapshot(hdfs, dir2, "s2"); + + // rename foo from dir2 to dir1 + final Path newfoo = new Path(dir1, foo.getName()); + hdfs.rename(foo, newfoo); + + // delete snapshot s1 + hdfs.deleteSnapshot(test, "s1"); + + // make sure the snapshot copy of file in s1 is merged to s0. For + // HDFS-4842, we need to make sure that we do not wrongly use s2 as the + // prior snapshot of s1. + final Path file_s2 = SnapshotTestHelper.getSnapshotPath(dir2, "s2", + "foo/bar/file"); + assertFalse(hdfs.exists(file_s2)); + final Path file_s0 = SnapshotTestHelper.getSnapshotPath(test, "s0", + "dir2/foo/bar/file"); + assertTrue(hdfs.exists(file_s0)); + + // check dir1: foo should be in the created list of s0 + INodeDirectoryWithSnapshot dir1Node = (INodeDirectoryWithSnapshot) fsdir + .getINode4Write(dir1.toString()); + List dir1DiffList = dir1Node.getDiffs().asList(); + assertEquals(1, dir1DiffList.size()); + List dList = dir1DiffList.get(0).getChildrenDiff() + .getList(ListType.DELETED); + assertTrue(dList.isEmpty()); + List cList = dir1DiffList.get(0).getChildrenDiff() + .getList(ListType.CREATED); + assertEquals(1, cList.size()); + INode cNode = cList.get(0); + INode fooNode = fsdir.getINode4Write(newfoo.toString()); + assertSame(cNode, fooNode); + + // check foo and its subtree + final Path newbar = new Path(newfoo, bar.getName()); + INodeDirectoryWithSnapshot barNode = (INodeDirectoryWithSnapshot) fsdir + .getINode4Write(newbar.toString()); + assertSame(fooNode.asDirectory(), barNode.getParent()); + // bar should only have a snapshot diff for s0 + List barDiffList = barNode.getDiffs().asList(); + assertEquals(1, barDiffList.size()); + DirectoryDiff diff = barDiffList.get(0); + assertEquals("s0", Snapshot.getSnapshotName(diff.snapshot)); + // and file should be stored in the deleted list of this snapshot diff + assertEquals("file", diff.getChildrenDiff().getList(ListType.DELETED) + .get(0).getLocalName()); + + // check dir2: a WithName instance for foo should be in the deleted list + // of the snapshot diff for s2 + INodeDirectoryWithSnapshot dir2Node = (INodeDirectoryWithSnapshot) fsdir + .getINode4Write(dir2.toString()); + List dir2DiffList = dir2Node.getDiffs().asList(); + // dir2Node should contain 2 snapshot diffs, one for s2, and the other was + // originally s1 (created when dir2 was transformed to a snapshottable dir), + // and currently is s0 + assertEquals(2, dir2DiffList.size()); + dList = dir2DiffList.get(1).getChildrenDiff().getList(ListType.DELETED); + assertEquals(1, dList.size()); + cList = dir2DiffList.get(0).getChildrenDiff().getList(ListType.CREATED); + assertTrue(cList.isEmpty()); + final Path foo_s2 = SnapshotTestHelper.getSnapshotPath(dir2, "s2", + foo.getName()); + INodeReference.WithName fooNode_s2 = + (INodeReference.WithName) fsdir.getINode(foo_s2.toString()); + assertSame(dList.get(0), fooNode_s2); + assertSame(fooNode.asReference().getReferredINode(), + fooNode_s2.getReferredINode()); + + restartClusterAndCheckImage(); + } }