ZOOKEEPER-3663: Clean Up ZNodeName Class

Author: David Mollitor <dmollitor@apache.org>

Reviewers: eolivelli@apache.org, andor@apache.org

Closes #1193 from belugabehr/ZOOKEEPER-3663

2 files changed, 194 insertions, 66 deletions

diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java b/zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
index 76b1759a8..24d6f13ed 100644
--- a/zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
+++ b/zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
@@ -18,45 +18,62 @@
 
 package org.apache.zookeeper.recipes.lock;
 
+import java.util.Objects;
+import java.util.Optional;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * Represents an ephemeral znode name which has an ordered sequence number
- * and can be sorted in order.
+ * Represents an immutable ephemeral znode name which has an ordered sequence
+ * number and can be sorted in order. The expected name format of the znode is
+ * as follows:
  *
+ * <pre>
+ * &lt;name&gt;-&lt;sequence&gt;
+ *
+ * For example: lock-00001
+ * </pre>
  */
 class ZNodeName implements Comparable<ZNodeName> {
 
-    private final String name;
-    private String prefix;
-    private int sequence = -1;
     private static final Logger LOG = LoggerFactory.getLogger(ZNodeName.class);
 
-    public ZNodeName(String name) {
-        if (name == null) {
-            throw new NullPointerException("id cannot be null");
-        }
-        this.name = name;
-        this.prefix = name;
-        int idx = name.lastIndexOf('-');
-        if (idx >= 0) {
+    private final String name;
+    private final String prefix;
+    private final Optional<Integer> sequence;
+
+    /**
+     * Instantiate a ZNodeName with the provided znode name.
+     *
+     * @param name The name of the znode
+     * @throws NullPointerException if {@code name} is {@code null}
+     */
+    public ZNodeName(final String name) {
+        this.name = Objects.requireNonNull(name, "ZNode name cannot be null");
+
+        final int idx = name.lastIndexOf('-');
+        if (idx < 0) {
+            this.prefix = name;
+            this.sequence = Optional.empty();
+        } else {
             this.prefix = name.substring(0, idx);
-            try {
-                this.sequence = Integer.parseInt(name.substring(idx + 1));
-                // If an exception occurred we mis-detected a sequence suffix,
-                // so return -1.
-            } catch (NumberFormatException e) {
-                LOG.warn("Number format exception for {}.", idx, e);
-            } catch (ArrayIndexOutOfBoundsException e) {
-                LOG.warn("Array out of bounds for {}.", idx, e);
-            }
+            this.sequence = Optional.ofNullable(parseSequenceString(name.substring(idx + 1)));
+        }
+    }
+
+    private Integer parseSequenceString(final String seq) {
+        try {
+            return Integer.parseInt(seq);
+        } catch (Exception e) {
+            LOG.warn("Number format exception for {}", seq, e);
+            return null;
         }
     }
 
     @Override
     public String toString() {
-        return name.toString();
+      return "ZNodeName [name=" + name + ", prefix=" + prefix + ", sequence="
+          + sequence + "]";
     }
 
     @Override
@@ -68,14 +85,14 @@ class ZNodeName implements Comparable<ZNodeName> {
             return false;
         }
 
-        ZNodeName sequence = (ZNodeName) o;
+        ZNodeName other = (ZNodeName) o;
 
-        return name.equals(sequence.name);
+        return name.equals(other.name);
     }
 
     @Override
     public int hashCode() {
-        return name.hashCode() + 37;
+        return name.hashCode();
     }
 
     /**
@@ -86,12 +103,18 @@ class ZNodeName implements Comparable<ZNodeName> {
      *         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.sequence - that.sequence;
-        if (answer == 0) {
-            return this.prefix.compareTo(that.prefix);
+    public int compareTo(final ZNodeName that) {
+        if (this.sequence.isPresent() && that.sequence.isPresent()) {
+            int cseq = Integer.compare(this.sequence.get(), that.sequence.get());
+            return (cseq != 0) ? cseq : this.prefix.compareTo(that.prefix);
+        }
+        if (this.sequence.isPresent()) {
+            return -1;
+        }
+        if (that.sequence.isPresent()) {
+            return 1;
         }
-        return answer;
+        return this.prefix.compareTo(that.prefix);
     }
 
     /**
@@ -102,9 +125,9 @@ class ZNodeName implements Comparable<ZNodeName> {
     }
 
     /**
-     * Returns the sequence number.
+     * Returns the optional sequence number.
      */
-    public int getZNodeName() {
+    public Optional<Integer> getSequence() {
         return sequence;
     }
 
diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/test/java/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java b/zookeeper-recipes/zookeeper-recipes-lock/src/test/java/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java
index 40872fca4..03dc2c29b 100644
--- a/zookeeper-recipes/zookeeper-recipes-lock/src/test/java/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java
+++ b/zookeeper-recipes/zookeeper-recipes-lock/src/test/java/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java
@@ -18,28 +18,85 @@
 
 package org.apache.zookeeper.recipes.lock;
 
-import java.util.SortedSet;
-import java.util.TreeSet;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.stream.Collectors;
 import org.junit.Assert;
 import org.junit.Test;
 
 /**
- * test for znodenames.
+ * Test for znodenames.
  */
 public class ZNodeNameTest {
 
     @Test
     public void testOrderWithSamePrefix() throws Exception {
-        String[] names = {"x-3", "x-5", "x-11", "x-1"};
-        String[] expected = {"x-1", "x-3", "x-5", "x-11"};
-        assertOrderedNodeNames(names, expected);
+        final String[] names = {"x-3", "x-5", "x-11", "x-1"};
+        ZNodeName zname;
+
+        final Collection<ZNodeName> nodeNames = Arrays.asList(names).stream()
+            .map(name -> new ZNodeName(name)).sorted().collect(Collectors.toList());
+
+        final Iterator<ZNodeName> it = nodeNames.iterator();
+
+        zname = it.next();
+        Assert.assertEquals("x-1", zname.getName());
+        Assert.assertEquals("x", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(1), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("x-3", zname.getName());
+        Assert.assertEquals("x", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(3), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("x-5", zname.getName());
+        Assert.assertEquals("x", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(5), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("x-11", zname.getName());
+        Assert.assertEquals("x", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(11), zname.getSequence().get());
     }
+
     @Test
     public void testOrderWithDifferentPrefixes() throws Exception {
-        String[] names = {"r-3", "r-2", "r-1", "w-2", "w-1"};
-        String[] expected = {"r-1", "w-1", "r-2", "w-2", "r-3"};
-        assertOrderedNodeNames(names, expected);
+        final String[] names = {"r-3", "r-2", "r-1", "w-2", "w-1"};
+        ZNodeName zname;
+
+        final Collection<ZNodeName> nodeNames = Arrays.asList(names).stream()
+            .map(name -> new ZNodeName(name)).sorted().collect(Collectors.toList());
+
+        final Iterator<ZNodeName> it = nodeNames.iterator();
+
+        zname = it.next();
+        Assert.assertEquals("r-1", zname.getName());
+        Assert.assertEquals("r", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(1), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("w-1", zname.getName());
+        Assert.assertEquals("w", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(1), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("r-2", zname.getName());
+        Assert.assertEquals("r", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(2), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("w-2", zname.getName());
+        Assert.assertEquals("w", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(2), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("r-3", zname.getName());
+        Assert.assertEquals("r", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(3), zname.getSequence().get());
     }
+
     @Test
     public void testOrderWithDifferentPrefixIncludingSessionId() throws Exception {
         String[] names = {
@@ -47,36 +104,84 @@ public class ZNodeNameTest {
             "x-170623981976748329-0000000003",
             "x-98566387950223723-0000000001"
         };
-        String[] expected = {
-            "x-98566387950223723-0000000001",
-            "x-242681582799028564-0000000002",
-            "x-170623981976748329-0000000003"
-        };
-        assertOrderedNodeNames(names, expected);
+        ZNodeName zname;
+
+        final Collection<ZNodeName> nodeNames = Arrays.asList(names).stream()
+            .map(name -> new ZNodeName(name)).sorted().collect(Collectors.toList());
+
+        final Iterator<ZNodeName> it = nodeNames.iterator();
+
+        zname = it.next();
+        Assert.assertEquals("x-98566387950223723-0000000001", zname.getName());
+        Assert.assertEquals("x-98566387950223723", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(1), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("x-242681582799028564-0000000002", zname.getName());
+        Assert.assertEquals("x-242681582799028564", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(2), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("x-170623981976748329-0000000003", zname.getName());
+        Assert.assertEquals("x-170623981976748329", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(3), zname.getSequence().get());
     }
+
     @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);
+        ZNodeName zname;
+
+        final Collection<ZNodeName> nodeNames = Arrays.asList(names).stream()
+            .map(name -> new ZNodeName(name)).sorted().collect(Collectors.toList());
+
+        final Iterator<ZNodeName> it = nodeNames.iterator();
+
+        zname = it.next();
+        Assert.assertEquals("r-2-2-1", zname.getName());
+        Assert.assertEquals("r-2-2", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(1), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("r-1-3-2", zname.getName());
+        Assert.assertEquals("r-1-3", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(2), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("r-3-1-3", zname.getName());
+        Assert.assertEquals("r-3-1", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(3), zname.getSequence().get());
+    }
+
+    @Test
+    public void testMissingSequenceNumber() throws Exception {
+        String[] names = {"c", "b-1", "a"};
+        ZNodeName zname;
+
+        final Collection<ZNodeName> nodeNames = Arrays.asList(names).stream()
+            .map(name -> new ZNodeName(name)).sorted().collect(Collectors.toList());
+
+        final Iterator<ZNodeName> it = nodeNames.iterator();
+
+        zname = it.next();
+        Assert.assertEquals("b-1", zname.getName());
+        Assert.assertEquals("b", zname.getPrefix());
+        Assert.assertEquals(Integer.valueOf(1), zname.getSequence().get());
+
+        zname = it.next();
+        Assert.assertEquals("a", zname.getName());
+        Assert.assertEquals("a", zname.getPrefix());
+        Assert.assertFalse(zname.getSequence().isPresent());
+
+        zname = it.next();
+        Assert.assertEquals("c", zname.getName());
+        Assert.assertEquals("c", zname.getPrefix());
+        Assert.assertFalse(zname.getSequence().isPresent());
     }
 
-    protected void assertOrderedNodeNames(String[] names, String[] expected) {
-        int size = names.length;
-        SortedSet<ZNodeName> nodeNames = new TreeSet<>();
-        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) {
-            String name = nodeName.getName();
-            Assert.assertEquals("Node " + index, expected[index++], name);
-        }
+    @Test(expected = NullPointerException.class)
+    public void testNullName() {
+        new ZNodeName(null);
     }
 
 }