You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zookeeper.apache.org by ph...@apache.org on 2017/11/15 23:24:00 UTC
zookeeper git commit: ZOOKEEPER-2931: WriteLock recipe: Fix bug in
znode ordering when the sessionId is part of the znode name
Repository: zookeeper
Updated Branches:
refs/heads/master f6d2abf58 -> f299303ad
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 <al...@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
Project: http://git-wip-us.apache.org/repos/asf/zookeeper/repo
Commit: http://git-wip-us.apache.org/repos/asf/zookeeper/commit/f299303a
Tree: http://git-wip-us.apache.org/repos/asf/zookeeper/tree/f299303a
Diff: http://git-wip-us.apache.org/repos/asf/zookeeper/diff/f299303a
Branch: refs/heads/master
Commit: f299303add79250ec2181f6c03b15e3754825284
Parents: f6d2abf
Author: Javier Cacheiro <al...@gmail.com>
Authored: Wed Nov 15 15:21:33 2017 -0800
Committer: Patrick Hunt <ph...@apache.org>
Committed: Wed Nov 15 15:21:33 2017 -0800
----------------------------------------------------------------------
.../apache/zookeeper/recipes/lock/ZNodeName.java | 16 +++++++++-------
.../zookeeper/recipes/lock/ZNodeNameTest.java | 16 ++++++++++++++--
2 files changed, 23 insertions(+), 9 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/zookeeper/blob/f299303a/src/recipes/lock/src/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
----------------------------------------------------------------------
diff --git a/src/recipes/lock/src/java/org/apache/zookeeper/recipes/lock/ZNodeName.java b/src/recipes/lock/src/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
index 99b6616..2e32e59 100644
--- a/src/recipes/lock/src/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
+++ b/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;
}
http://git-wip-us.apache.org/repos/asf/zookeeper/blob/f299303a/src/recipes/lock/test/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java
----------------------------------------------------------------------
diff --git a/src/recipes/lock/test/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java b/src/recipes/lock/test/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java
index 275b9a5..7281384 100644
--- a/src/recipes/lock/test/org/apache/zookeeper/recipes/lock/ZNodeNameTest.java
+++ b/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) {