From 60080fbfcf3919fc155af45975fa24bdeb6c8ffe Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Fri, 2 Mar 2018 10:51:05 -0800 Subject: [PATCH] HDFS-13171. Handle Deletion of nodes in SnasphotSkipList. Contributed by Shashikant Banerjee --- .../namenode/snapshot/DirectoryDiffList.java | 80 +++++++++++--- .../snapshot/TestDirectoryDiffList.java | 102 ++++++++++++++---- 2 files changed, 149 insertions(+), 33 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryDiffList.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryDiffList.java index d2b20d3d4e..58930e27d8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryDiffList.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryDiffList.java @@ -22,6 +22,8 @@ DirectoryWithSnapshotFeature.DirectoryDiff; import org.apache.hadoop.hdfs.server.namenode.snapshot. DirectoryWithSnapshotFeature.ChildrenDiff; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.List; import java.util.ArrayList; @@ -67,6 +69,8 @@ * Once a snapshot gets deleted, the list needs to be balanced. */ public class DirectoryDiffList implements DiffList { + public static final Logger LOG = + LoggerFactory.getLogger(DirectoryDiffList.class); private static class SkipDiff { /** @@ -100,7 +104,7 @@ public void setDiff(ChildrenDiff diff) { @Override public String toString() { - return "->" + skipTo; + return "->" + skipTo + (diff == null? " (diff==null)": ""); } } @@ -138,6 +142,13 @@ public int level() { return skipDiffList.size() - 1; } + void trim() { + for (int level = level(); + level > 0 && getSkipNode(level) == null; level--) { + skipDiffList.remove(level); + } + } + public DirectoryDiff getDiff() { return diff; } @@ -281,6 +292,23 @@ public void addFirst(DirectoryDiff diff) { skipNodeList.add(0, newNode); } + private SkipListNode[] findPreviousNodes(SkipListNode node, int nodeLevel) { + final SkipListNode[] nodePath = new SkipListNode[nodeLevel + 1]; + SkipListNode cur = head; + final int headLevel = head.level(); + for (int level = headLevel < nodeLevel ? headLevel : nodeLevel; + level >= 0; level--) { + while (cur.getSkipNode(level) != node) { + cur = cur.getSkipNode(level); + } + nodePath[level] = cur; + } + for (int level = headLevel + 1; level <= nodeLevel; level++) { + nodePath[level] = head; + } + return nodePath; + } + /** * Adds the specified data element to the end of the SkipList, * if the element is not already present. @@ -289,16 +317,8 @@ public void addFirst(DirectoryDiff diff) { @Override public boolean addLast(DirectoryDiff diff) { final int nodeLevel = randomLevel(skipInterval, maxSkipLevels); - final SkipListNode[] nodePath = new SkipListNode[nodeLevel + 1]; - SkipListNode cur = head; final int headLevel = head.level(); - for (int level = headLevel < nodeLevel ? headLevel : nodeLevel; - level >= 0; level--) { - while (cur.getSkipNode(level) != null) { - cur = cur.getSkipNode(level); - } - nodePath[level] = cur; - } + final SkipListNode[] nodePath = findPreviousNodes(null, nodeLevel); for (int level = headLevel + 1; level <= nodeLevel; level++) { head.skipDiffList.add(new SkipDiff(null)); nodePath[level] = head; @@ -327,6 +347,7 @@ public boolean addLast(DirectoryDiff diff) { } } nodePath[level].setSkipTo(current, level); + current.setSkipTo(null, level); } return skipNodeList.add(current); } @@ -366,11 +387,44 @@ public DirectoryDiff get(int index) { * Removes the element at the specified position in this list. * * @param index the index of the element to be removed - * @throws UnsupportedOperationException {@inheritDoc} + * @return the removed DirectoryDiff */ @Override public DirectoryDiff remove(int index) { - throw new UnsupportedOperationException(); + SkipListNode node = getNode(index); + int headLevel = head.level(); + int nodeLevel = node.level(); + final SkipListNode[] nodePath = findPreviousNodes(node, nodeLevel); + for (int level = 0; level <= nodeLevel; level++) { + if (nodePath[level] != head && level > 0) { + // if the last snapshot is deleted, for all the skip level nodes + // pointing to the last one, the combined children diff at each level + // > 0 should be made null and skip pointers will be updated to null. + // if the snapshot being deleted is not the last one, we have to merge + // the diff of deleted node at each level to the previous skip level + // node at that level and the skip pointers will be updated to point to + // the skip nodes of the deleted node. + if (index == size() - 1) { + nodePath[level].setSkipDiff(null, level); + } else { + /* Ideally at level 0, the deleted diff will be combined with + * the previous diff , and deleted inodes will be cleaned up + * by passing a deleted processor here while combining the diffs. + * Level 0 merge with previous diff will be handled inside the + * {@link AbstractINodeDiffList#deleteSnapshotDiff} function. + */ + if (node.getChildrenDiff(level) != null) { + nodePath[level].getChildrenDiff(level) + .combinePosterior(node.getChildrenDiff(level), null); + } + } + } + nodePath[level].setSkipTo(node.getSkipNode(level), level); + } + if (nodeLevel == headLevel) { + head.trim(); + } + return skipNodeList.remove(index).getDiff(); } /** @@ -482,7 +536,7 @@ public List getMinListForRange(int fromIndex, int toIndex, @Override public String toString() { final StringBuilder b = new StringBuilder(getClass().getSimpleName()); - b.append("\nhead: ").append(head).append(head.skipDiffList); + b.append(" head: ").append(head).append(head.skipDiffList); for (SkipListNode n : skipNodeList) { b.append("\n ").append(n).append(n.skipDiffList); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestDirectoryDiffList.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestDirectoryDiffList.java index 99b6da2aeb..e493e4b4af 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestDirectoryDiffList.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestDirectoryDiffList.java @@ -35,11 +35,14 @@ import java.util.Collections; import java.util.List; +import java.util.concurrent.ThreadLocalRandom; +import java.util.function.IntFunction; /** * This class tests the DirectoryDiffList API's. */ public class TestDirectoryDiffList{ + static final int NUM_SNAPSHOTS = 100; static { SnapshotTestHelper.disableLogs(); } @@ -78,15 +81,15 @@ static void assertList(List expected, List computed) { static void verifyChildrenList(DirectoryDiffList skip, INodeDirectory dir) { final int n = skip.size(); for (int i = 0; i < skip.size(); i++) { - final List expected = - ReadOnlyList.Util.asList(dir.getChildrenList(i)); + final List expected = ReadOnlyList.Util.asList( + dir.getChildrenList(dir.getDiffs().asList().get(i).getSnapshotId())); final List computed = getChildrenList(skip, i, n, dir); try { assertList(expected, computed); } catch (AssertionError ae) { throw new AssertionError( "i = " + i + "\ncomputed = " + computed + "\nexpected = " - + expected, ae); + + expected + "\n" + skip, ae); } } } @@ -136,28 +139,16 @@ static Path getChildPath(Path parent, int i) { @Test public void testAddLast() throws Exception { - testAddLast(7); + testAddLast(NUM_SNAPSHOTS); } static void testAddLast(int n) throws Exception { final Path root = new Path("/testAddLast" + n); - hdfs.mkdirs(root); - SnapshotTestHelper.createSnapshot(hdfs, root, "s0"); - for (int i = 1; i < n; i++) { - final Path child = getChildPath(root, i); - hdfs.mkdirs(child); - SnapshotTestHelper.createSnapshot(hdfs, root, "s" + i); - } - INodeDirectory dir = fsdir.getINode(root.toString()).asDirectory(); - DiffList diffs = dir.getDiffs().asList(); + DirectoryDiffList.LOG.info("run " + root); final DirectoryDiffList skipList = new DirectoryDiffList(0, 3, 5); final DiffList arrayList = new DiffListByArrayList<>(0); - for (DirectoryDiff d : diffs) { - skipList.addLast(d); - arrayList.addLast(d); - } - + INodeDirectory dir = addDiff(n, skipList, arrayList, root); // verify that the both the children list obtained from hdfs and // DirectoryDiffList are same verifyChildrenList(skipList, dir); @@ -167,11 +158,13 @@ static void testAddLast(int n) throws Exception { @Test public void testAddFirst() throws Exception { - testAddFirst(7); + testAddFirst(NUM_SNAPSHOTS); } static void testAddFirst(int n) throws Exception { final Path root = new Path("/testAddFirst" + n); + DirectoryDiffList.LOG.info("run " + root); + hdfs.mkdirs(root); for (int i = 1; i < n; i++) { final Path child = getChildPath(root, i); @@ -182,7 +175,7 @@ static void testAddFirst(int n) throws Exception { for (int i = 1; i < n; i++) { final Path child = getChildPath(root, n - i); hdfs.delete(child, false); - SnapshotTestHelper.createSnapshot(hdfs, root, "s" + i); + hdfs.createSnapshot(root, "s" + i); } DiffList diffs = dir.getDiffs().asList(); List childrenList = ReadOnlyList.Util.asList(dir.getChildrenList( @@ -199,4 +192,73 @@ static void testAddFirst(int n) throws Exception { verifyChildrenList(skipList, dir); verifyChildrenList(arrayList, skipList, dir, childrenList); } + + static INodeDirectory addDiff(int n, DiffList skipList, DiffList arrayList, + final Path root) throws Exception { + hdfs.mkdirs(root); + SnapshotTestHelper.createSnapshot(hdfs, root, "s0"); + for (int i = 1; i < n; i++) { + final Path child = getChildPath(root, i); + hdfs.mkdirs(child); + hdfs.createSnapshot(root, "s" + i); + } + INodeDirectory dir = fsdir.getINode(root.toString()).asDirectory(); + DiffList diffs = dir.getDiffs().asList(); + for (DirectoryDiff d : diffs) { + skipList.addLast(d); + arrayList.addLast(d); + } + return dir; + } + + @Test + public void testRemoveFromTail() throws Exception { + final int n = NUM_SNAPSHOTS; + testRemove("FromTail", n, i -> n - 1 - i); + } + + @Test + public void testReomveFromHead() throws Exception { + testRemove("FromHead", NUM_SNAPSHOTS, i -> 0); + } + + @Test + public void testRemoveRandom() throws Exception { + final int n = NUM_SNAPSHOTS; + testRemove("Random", n, i -> ThreadLocalRandom.current().nextInt(n - i)); + } + + static void testRemove(String name, int n, IntFunction indexFunction) + throws Exception { + final Path root = new Path("/testRemove" + name + n); + DirectoryDiffList.LOG.info("run " + root); + + final DirectoryDiffList skipList = new DirectoryDiffList(0, 3, 5); + final DiffList arrayList = new DiffListByArrayList<>(0); + final INodeDirectory dir = addDiff(n, skipList, arrayList, root); + Assert.assertEquals(n, arrayList.size()); + Assert.assertEquals(n, skipList.size()); + + for(int i = 0; i < n; i++) { + final int index = indexFunction.apply(i); + final DirectoryDiff diff = remove(index, skipList, arrayList); + hdfs.deleteSnapshot(root, "s" + diff.getSnapshotId()); + verifyChildrenList(skipList, dir); + verifyChildrenList(arrayList, skipList, dir, Collections.emptyList()); + } + } + + static DirectoryDiff remove(int i, DirectoryDiffList skip, + DiffList array) { + DirectoryDiffList.LOG.info("remove " + i); + final DirectoryDiff expected = array.remove(i); + final DirectoryDiff computed = skip.remove(i); + assertDirectoryDiff(expected, computed); + return expected; + } + + static void assertDirectoryDiff(DirectoryDiff expected, + DirectoryDiff computed) { + Assert.assertEquals(expected.getSnapshotId(), computed.getSnapshotId()); + } } \ No newline at end of file