ZOOKEEPER-2931: WriteLock recipe: Fix bug in znode ordering when the sessionId is part of the znode name

This PR solves a bug in the WriteLock recipe related to the ordering the znodes.

In the original version when the nodes are sorted in WriteLock.java using a TreeSet the whole znode path is taken into account and not just the sequence number.

This causes an issue when the sessionId is included in the znode path because a znode with a lower sessionId will appear as lower than other znode with a higher sessionId even if its sequence number is bigger. In specific situations this ended with two clients holding the lock at the same time.

Author: Javier Cacheiro <alcachi@gmail.com>

Reviewers: phunt@apache.org

Closes #413 from javicacheiro/fix_writelock and squashes the following commits:

ed0e6648 [Javier Cacheiro] Secondary sort znodes with the same sequence number by prefix
b382648d [Javier Cacheiro] Add unit tests
f9c581d4 [Javier Cacheiro] Fix bug in znode ordering when the sessionId is part of the znode name

Change-Id: Id384ce6079bbdfef10054fabb8bb93378d86024f

src/recipes/lock/src/java/org/apache/zookeeper/recipes/lock/ZNodeName.java

@@ -74,15 +74,17 @@ class ZNodeName implements Comparable<ZNodeName> {
         return name.hashCode() + 37;
     }
 
+    /**
+     * Compare znodes based on their sequence number
+     * @param that other znode to compare to
+     * @return the difference between their sequence numbers: a positive value if this
+     *         znode has a larger sequence number, 0 if they have the same sequence number
+     *         or a negative number if this znode has a lower sequence number
+     */
     public int compareTo(ZNodeName that) {
-        int answer = this.prefix.compareTo(that.prefix);
+        int answer = this.sequence - that.sequence;
         if (answer == 0) {
-            int s1 = this.sequence;
-            int s2 = that.sequence;
-            if (s1 == -1 && s2 == -1) {
-                return this.name.compareTo(that.name);
-            }
-            answer = s1 == -1 ? 1 : s2 == -1 ? -1 : s1 - s2;
+            return this.prefix.compareTo(that.prefix);
         }
         return answer;
     }

src/recipes/lock/test/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java

@@ -37,17 +37,29 @@ public class ZNodeNameTest {
     @Test
     public void testOrderWithDifferentPrefixes() throws Exception {
         String[] names = { "r-3", "r-2", "r-1", "w-2", "w-1" };
-        String[] expected = { "r-1", "r-2", "r-3", "w-1", "w-2" };
+        String[] expected = { "r-1", "w-1", "r-2", "w-2", "r-3" };
+        assertOrderedNodeNames(names, expected);
+    }
+    @Test
+    public void testOrderWithDifferentPrefixIncludingSessionId() throws Exception {
+        String[] names = { "x-242681582799028564-0000000002", "x-170623981976748329-0000000003", "x-98566387950223723-0000000001" };
+        String[] expected = { "x-98566387950223723-0000000001", "x-242681582799028564-0000000002", "x-170623981976748329-0000000003" };
+        assertOrderedNodeNames(names, expected);
+    }
+    @Test
+    public void testOrderWithExtraPrefixes() throws Exception {
+        String[] names = { "r-1-3-2", "r-2-2-1", "r-3-1-3" };
+        String[] expected = { "r-2-2-1", "r-1-3-2", "r-3-1-3" };
         assertOrderedNodeNames(names, expected);
     }
 
     protected void assertOrderedNodeNames(String[] names, String[] expected) {
         int size = names.length;
-        Assert.assertEquals("The two arrays should be the same size!", names.length, expected.length);
         SortedSet<ZNodeName> nodeNames = new TreeSet<ZNodeName>();
         for (String name : names) {
             nodeNames.add(new ZNodeName(name));
         }
+        Assert.assertEquals("The SortedSet does not have the expected size!", nodeNames.size(), expected.length);
 
         int index = 0;
         for (ZNodeName nodeName : nodeNames) {