From 91e3a643ff6c562d30e75a61ed2d626befbcc40c Mon Sep 17 00:00:00 2001 From: nickhill Date: Fri, 2 Nov 2018 16:51:15 -0700 Subject: [PATCH 1/5] fix path used when re-creating sequential PersistentNode with protection would previously result in creation of a second non-sequential znode --- .../curator/framework/recipes/nodes/PersistentNode.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java index bdf607cd9f..9c9b527c6c 100644 --- a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java +++ b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java @@ -445,7 +445,8 @@ private void createNode() try { String existingPath = nodePath.get(); - String createPath = (existingPath != null && !useProtection) ? existingPath : basePath; + String createPath = existingPath == null || (useProtection && !isSequential(mode)) ? basePath + : (useProtection ? basePath + existingPath.substring(existingPath.length()-10) : existingPath); CreateModable> localCreateMethod = createMethod.get(); if ( localCreateMethod == null ) @@ -463,6 +464,10 @@ private void createNode() throw new RuntimeException("Creating node. BasePath: " + basePath, e); // should never happen unless there's a programming error - so throw RuntimeException } } + + private static boolean isSequential(CreateMode mode) { + return mode == CreateMode.EPHEMERAL_SEQUENTIAL || mode == CreateMode.PERSISTENT_SEQUENTIAL; + } private CreateMode getCreateMode(boolean pathIsSet) { From 8e4dba7a070beaf110267843ba0611ee7cae25fc Mon Sep 17 00:00:00 2001 From: nickhill Date: Mon, 5 Nov 2018 09:57:29 -0800 Subject: [PATCH 2/5] add targeted unit test for bug/fix --- .../recipes/nodes/TestPersistentNode.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java index 0fdd7c8aa5..b157000bae 100644 --- a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java +++ b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java @@ -28,6 +28,8 @@ import org.apache.zookeeper.CreateMode; import org.testng.Assert; import org.testng.annotations.Test; + +import java.util.List; import java.util.concurrent.TimeUnit; public class TestPersistentNode extends BaseClassForTests @@ -138,4 +140,35 @@ public void testQuickCloseNodeExists() throws Exception CloseableUtils.closeQuietly(client); } } + + @Test + public void testEphemeralSequentialWithProtectionReconnection() throws Exception + { + Timing timing = new Timing(); + PersistentNode pen = null; + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(), timing.connection(), new RetryOneTime(1)); + try + { + client.start(); + client.create().creatingParentsIfNeeded().forPath("/test/one"); + + pen = new PersistentNode(client, CreateMode.EPHEMERAL_SEQUENTIAL, true, "/test/one/two", new byte[0]); + pen.start(); + List children = client.getChildren().forPath("/test/one"); + System.out.println("children before restart: "+children); + Assert.assertEquals(1, children.size()); + server.stop(); + timing.sleepABit(); + server.restart(); + timing.sleepABit(); + List childrenAfter = client.getChildren().forPath("/test/one"); + System.out.println("children after restart: "+childrenAfter); + Assert.assertEquals(children, childrenAfter, "unexpected znodes: "+childrenAfter); + } + finally + { + CloseableUtils.closeQuietly(pen); + CloseableUtils.closeQuietly(client); + } + } } From 824b72729a6168ce9775ccd86b88a08656dbc064 Mon Sep 17 00:00:00 2001 From: nickhill Date: Tue, 6 Nov 2018 17:32:23 -0800 Subject: [PATCH 3/5] change to use if statements; remove redundant isSequential method and define SEQUENTIAL_SUFFIX_DIGITS int constant --- .../recipes/nodes/PersistentNode.java | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java index 9c9b527c6c..5fcbc645f8 100644 --- a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java +++ b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java @@ -422,6 +422,9 @@ protected void deleteNode() throws Exception } } + // Hardcoded in {@link org.apache.zookeeper.server.PrepRequestProcessor} + static final int SEQUENTIAL_SUFFIX_DIGITS = 10; + private void createNode() { if ( !isActive() ) @@ -444,9 +447,19 @@ private void createNode() try { - String existingPath = nodePath.get(); - String createPath = existingPath == null || (useProtection && !isSequential(mode)) ? basePath - : (useProtection ? basePath + existingPath.substring(existingPath.length()-10) : existingPath); + String existingPath = nodePath.get(), createPath; + if ( existingPath != null && !useProtection ) + { + createPath = existingPath; + } + else + { + createPath = basePath; + if ( useProtection && mode.isSequential() ) + { + createPath += existingPath.substring(existingPath.length()-SEQUENTIAL_SUFFIX_DIGITS); + } + } CreateModable> localCreateMethod = createMethod.get(); if ( localCreateMethod == null ) @@ -464,10 +477,6 @@ private void createNode() throw new RuntimeException("Creating node. BasePath: " + basePath, e); // should never happen unless there's a programming error - so throw RuntimeException } } - - private static boolean isSequential(CreateMode mode) { - return mode == CreateMode.EPHEMERAL_SEQUENTIAL || mode == CreateMode.PERSISTENT_SEQUENTIAL; - } private CreateMode getCreateMode(boolean pathIsSet) { From 4d6069a03cf6ff3020abc6faa137515772bd7002 Mon Sep 17 00:00:00 2001 From: nickhill Date: Wed, 7 Nov 2018 08:52:40 -0800 Subject: [PATCH 4/5] fix bug in reworked createPath determination logic --- .../framework/recipes/nodes/PersistentNode.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java index 5fcbc645f8..80b07e8df5 100644 --- a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java +++ b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java @@ -452,13 +452,13 @@ private void createNode() { createPath = existingPath; } - else + else if ( existingPath == null || !mode.isSequential() ) { createPath = basePath; - if ( useProtection && mode.isSequential() ) - { - createPath += existingPath.substring(existingPath.length()-SEQUENTIAL_SUFFIX_DIGITS); - } + } + else + { + createPath = basePath + existingPath.substring(existingPath.length() - SEQUENTIAL_SUFFIX_DIGITS); } CreateModable> localCreateMethod = createMethod.get(); From eebff920c99f1d8d3d7527deed9958bb4595e59a Mon Sep 17 00:00:00 2001 From: nickhill Date: Thu, 15 Nov 2018 10:19:51 -0800 Subject: [PATCH 5/5] Move sequential suffix extraction logic into ZKPaths helper method --- .../java/org/apache/curator/utils/ZKPaths.java | 15 +++++++++++++++ .../framework/recipes/nodes/PersistentNode.java | 10 ++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java b/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java index 9d6ea9d443..8e94d2b652 100644 --- a/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java +++ b/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java @@ -177,6 +177,21 @@ public static PathAndNode getPathAndNode(String path) return new PathAndNode(parentPath, node); } + // Hardcoded in {@link org.apache.zookeeper.server.PrepRequestProcessor} + static final int SEQUENTIAL_SUFFIX_DIGITS = 10; + + /** + * Extracts the ten-digit suffix from a sequential znode path. Does not currently perform validation on the + * provided path; it will just return a string comprising the last ten characters. + * + * @param path the path of a sequential znodes + * @return the sequential suffix + */ + public static String extractSequentialSuffix(String path) { + int length = path.length(); + return length > SEQUENTIAL_SUFFIX_DIGITS ? path.substring(length - SEQUENTIAL_SUFFIX_DIGITS) : path; + } + private static final Splitter PATH_SPLITTER = Splitter.on(PATH_SEPARATOR).omitEmptyStrings(); /** diff --git a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java index 80b07e8df5..81e8dd9f1a 100644 --- a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java +++ b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java @@ -37,6 +37,7 @@ import org.apache.curator.framework.state.ConnectionStateListener; import org.apache.curator.utils.PathUtils; import org.apache.curator.utils.ThreadUtils; +import org.apache.curator.utils.ZKPaths; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.WatchedEvent; @@ -422,9 +423,6 @@ protected void deleteNode() throws Exception } } - // Hardcoded in {@link org.apache.zookeeper.server.PrepRequestProcessor} - static final int SEQUENTIAL_SUFFIX_DIGITS = 10; - private void createNode() { if ( !isActive() ) @@ -452,13 +450,13 @@ private void createNode() { createPath = existingPath; } - else if ( existingPath == null || !mode.isSequential() ) + else if ( existingPath != null && mode.isSequential() ) { - createPath = basePath; + createPath = basePath + ZKPaths.extractSequentialSuffix(existingPath); } else { - createPath = basePath + existingPath.substring(existingPath.length() - SEQUENTIAL_SUFFIX_DIGITS); + createPath = basePath; } CreateModable> localCreateMethod = createMethod.get();