From 8ca8687fb2fc74ab7c5199a93c70661996ad9e72 Mon Sep 17 00:00:00 2001 From: Tsz-wo Sze Date: Thu, 15 Nov 2012 21:34:54 +0000 Subject: [PATCH] HDFS-4188. Add Snapshot.ID_COMPARATOR for comparing IDs and fix a bug in ReadOnlyList.Util.binarySearch(..). git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-2802@1410027 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-hdfs/CHANGES.HDFS-2802.txt | 3 ++ .../hadoop/hdfs/server/namenode/INode.java | 7 +--- .../hdfs/server/namenode/INodeDirectory.java | 2 +- .../snapshot/INodeDirectoryWithSnapshot.java | 10 ++---- .../server/namenode/snapshot/Snapshot.java | 20 ++++++++--- .../apache/hadoop/hdfs/util/ReadOnlyList.java | 20 ++++++++++- .../TestINodeDirectoryWithSnapshot.java | 33 ++++++++++++++++++- 7 files changed, 73 insertions(+), 22 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt index 868b4402ba..d543673678 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.HDFS-2802.txt @@ -70,3 +70,6 @@ Branch-2802 Snapshot (Unreleased) HDFS-4148. Disallow write/modify operations on files and directories in a snapshot. (Brandon Li via suresh) + + HDFS-4188. Add Snapshot.ID_COMPARATOR for comparing IDs and fix a bug in + ReadOnlyList.Util.binarySearch(..). (szetszwo) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java index 96614cf63e..a2ed343ffa 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java @@ -19,12 +19,9 @@ import java.io.PrintWriter; import java.io.StringWriter; -import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; -import java.util.List; import java.util.Map; import org.apache.hadoop.classification.InterfaceAudience; @@ -49,10 +46,8 @@ */ @InterfaceAudience.Private public abstract class INode implements Comparable { - static final List EMPTY_LIST - = Collections.unmodifiableList(new ArrayList()); static final ReadOnlyList EMPTY_READ_ONLY_LIST - = ReadOnlyList.Util.asReadOnlyList(EMPTY_LIST); + = ReadOnlyList.Util.emptyList(); /** * The inode name is in java UTF8 encoding; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java index 40a2e72e62..7bf13e7f1c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java @@ -586,7 +586,7 @@ private void setSnapshot(Snapshot s) { } private void updateLatestSnapshot(Snapshot s) { - if (snapshot == null || snapshot.compareTo(s) < 0) { + if (Snapshot.ID_COMPARATOR.compare(snapshot, s) < 0) { snapshot = s; } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java index bc6fa4c496..d11995e45d 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/INodeDirectoryWithSnapshot.java @@ -79,17 +79,11 @@ private static int search(final List inodes, final byte[] name) { return inodes == null? -1: Collections.binarySearch(inodes, name); } - /** The ID (e.g. snapshot ID) of this object. */ - final int id; /** c-list: inode(s) created in current. */ private List created; /** d-list: inode(s) deleted from current. */ private List deleted; - Diff(int id) { - this.id = id; - } - /** * Insert the inode to created. * @param i the insertion point defined @@ -251,7 +245,7 @@ static String toString(List inodes) { @Override public String toString() { - return getClass().getSimpleName() + "_" + id + return getClass().getSimpleName() + ":\n created=" + toString(created) + "\n deleted=" + toString(deleted); } @@ -261,4 +255,4 @@ public String toString() { super(name, dir.getPermissionStatus()); parent = dir; } -} \ No newline at end of file +} diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java index 6f8e18d645..654e9fbbcd 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java @@ -17,11 +17,26 @@ */ package org.apache.hadoop.hdfs.server.namenode.snapshot; +import java.util.Comparator; + import org.apache.hadoop.classification.InterfaceAudience; /** Snapshot of a sub-tree in the namesystem. */ @InterfaceAudience.Private public class Snapshot implements Comparable { + /** Compare snapshot IDs with null <= s for any snapshot s. */ + public static final Comparator ID_COMPARATOR + = new Comparator() { + @Override + public int compare(Snapshot left, Snapshot right) { + if (left == null) { + return right == null? 0: -1; + } else { + return right == null? 1: left.id - right.id; + } + } + }; + /** Snapshot ID. */ private final int id; /** The root directory of the snapshot. */ @@ -41,11 +56,6 @@ public INodeDirectoryWithSnapshot getRoot() { public int compareTo(byte[] bytes) { return root.compareTo(bytes); } - - /** Compare snapshot IDs. */ - public int compareTo(Snapshot s) { - return id - s.id; - } @Override public String toString() { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/ReadOnlyList.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/ReadOnlyList.java index c7f36da321..6da7dbabff 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/ReadOnlyList.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/ReadOnlyList.java @@ -52,6 +52,11 @@ public interface ReadOnlyList extends Iterable { * Utilities for {@link ReadOnlyList} */ public static class Util { + /** @return an empty list. */ + public static ReadOnlyList emptyList() { + return ReadOnlyList.Util.asReadOnlyList(Collections.emptyList()); + } + /** * The same as {@link Collections#binarySearch(List, Object)} * except that the list is a {@link ReadOnlyList}. @@ -61,7 +66,20 @@ public static class Util { */ public static > int binarySearch( final ReadOnlyList list, final K key) { - return Collections.binarySearch(asList(list), key); + int lower = 0; + for(int upper = list.size() - 1; lower <= upper; ) { + final int mid = (upper + lower) >>> 1; + + final int d = list.get(mid).compareTo(key); + if (d == 0) { + return mid; + } else if (d > 0) { + upper = mid - 1; + } else { + lower = mid + 1; + } + } + return -(lower + 1); } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java index f72ceb5d16..1e8ac0f398 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestINodeDirectoryWithSnapshot.java @@ -81,7 +81,7 @@ void runDiffTest(int startSize, int numModifications, boolean computeDiff) { // make modifications to current and record the diff final List current = new ArrayList(previous); - final Diff diff = computeDiff? new Diff(0): null; + final Diff diff = computeDiff? new Diff(): null; for(int m = 0; m < numModifications; m++) { // if current is empty, the next operation must be create; @@ -228,4 +228,35 @@ static void modify(INode inode, final List current, Diff diff) { diff.modify(oldinode, newinode); } } + + /** + * Test {@link Snapshot#ID_COMPARATOR}. + */ + @Test + public void testIdCmp() { + final INodeDirectory dir = new INodeDirectory("foo", PERM); + final INodeDirectorySnapshottable snapshottable + = INodeDirectorySnapshottable.newInstance(dir, 100); + final Snapshot[] snapshots = { + new Snapshot(1, "s1", snapshottable), + new Snapshot(1, "s1", snapshottable), + new Snapshot(2, "s2", snapshottable), + new Snapshot(2, "s2", snapshottable), + }; + + Assert.assertEquals(0, Snapshot.ID_COMPARATOR.compare(null, null)); + for(Snapshot s : snapshots) { + Assert.assertTrue(Snapshot.ID_COMPARATOR.compare(null, s) < 0); + Assert.assertTrue(Snapshot.ID_COMPARATOR.compare(s, null) > 0); + + for(Snapshot t : snapshots) { + final int expected = s.getRoot().getLocalName().compareTo( + t.getRoot().getLocalName()); + final int computed = Snapshot.ID_COMPARATOR.compare(s, t); + Assert.assertEquals(expected > 0, computed > 0); + Assert.assertEquals(expected == 0, computed == 0); + Assert.assertEquals(expected < 0, computed < 0); + } + } + } }