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

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<byte[]> {
-  static final List<INode> EMPTY_LIST
-      = Collections.unmodifiableList(new ArrayList<INode>());
   static final ReadOnlyList<INode> EMPTY_READ_ONLY_LIST
-      = ReadOnlyList.Util.asReadOnlyList(EMPTY_LIST);
+      = ReadOnlyList.Util.emptyList();
 
   /**
    *  The inode name is in java UTF8 encoding; 

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

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<INode> 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<INode> created;
     /** d-list: inode(s) deleted from current. */
     private List<INode> 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<INode> 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;
   }
-}
+}

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<byte[]> {
+  /** Compare snapshot IDs with null <= s for any snapshot s. */
+  public static final Comparator<Snapshot> ID_COMPARATOR
+      = new Comparator<Snapshot>() {
+    @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() {

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/ReadOnlyList.java

@@ -52,6 +52,11 @@ public interface ReadOnlyList<E> extends Iterable<E> {
    * Utilities for {@link ReadOnlyList}
    */
   public static class Util {
+    /** @return an empty list. */
+    public static <E> ReadOnlyList<E> emptyList() {
+      return ReadOnlyList.Util.asReadOnlyList(Collections.<E>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 <K, E extends Comparable<K>> int binarySearch(
         final ReadOnlyList<E> 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);
     }
 
     /**

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<INode> current = new ArrayList<INode>(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<INode> 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);
+      }
+    }
+  }
 }