Repository: curator Updated Branches: refs/heads/CURATOR-42 [created] b174dfb14
CURATOR-42 - Modified guaranteed delete handling so that it will only add the node to the guaranteed delete set if a recoverable exception is encountered. If a non recoverable exception (such as a NoNodeException) is encountered, then this will not be retried. Added a debug listener to the FailedDeleteManager to facilitate unit testing this case. Added unit tests, for both guaranteed deletes in the background and foreground for the NoNodeException case. Note that this error was only present for guaranteed deletes in the foreground. Project: http://git-wip-us.apache.org/repos/asf/curator/repo Commit: http://git-wip-us.apache.org/repos/asf/curator/commit/b174dfb1 Tree: http://git-wip-us.apache.org/repos/asf/curator/tree/b174dfb1 Diff: http://git-wip-us.apache.org/repos/asf/curator/diff/b174dfb1 Branch: refs/heads/CURATOR-42 Commit: b174dfb145ebaea51bcd348cec950997725e7b3f Parents: 9c0a617 Author: Cameron McKenzie <[email protected]> Authored: Wed Jul 30 08:06:25 2014 +1000 Committer: Cameron McKenzie <[email protected]> Committed: Wed Jul 30 08:06:25 2014 +1000 ---------------------------------------------------------------------- .../framework/imps/DeleteBuilderImpl.java | 10 +-- .../framework/imps/FailedDeleteManager.java | 13 +++ .../framework/imps/TestFailedDeleteManager.java | 86 ++++++++++++++++++++ 3 files changed, 102 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/curator/blob/b174dfb1/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java ---------------------------------------------------------------------- diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java index 319fce2..5d8b846 100644 --- a/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java +++ b/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java @@ -27,7 +27,6 @@ import org.apache.curator.framework.api.ChildrenDeletable; import org.apache.curator.framework.api.CuratorEvent; import org.apache.curator.framework.api.CuratorEventType; import org.apache.curator.framework.api.DeleteBuilder; -import org.apache.curator.framework.api.Guaranteeable; import org.apache.curator.framework.api.Pathable; import org.apache.curator.framework.api.transaction.CuratorTransactionBridge; import org.apache.curator.framework.api.transaction.OperationType; @@ -248,14 +247,11 @@ class DeleteBuilderImpl implements DeleteBuilder, BackgroundOperation<String> } } ); - } - catch ( KeeperException.NodeExistsException e ) - { - throw e; - } + } catch ( Exception e ) { - if ( guaranteed ) + //Only retry a guaranteed delete if it's a retryable error + if( RetryLoop.isRetryException(e) && guaranteed ) { client.getFailedDeleteManager().addFailedDelete(unfixedPath); } http://git-wip-us.apache.org/repos/asf/curator/blob/b174dfb1/curator-framework/src/main/java/org/apache/curator/framework/imps/FailedDeleteManager.java ---------------------------------------------------------------------- diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/FailedDeleteManager.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/FailedDeleteManager.java index d9f1771..f02e852 100644 --- a/curator-framework/src/main/java/org/apache/curator/framework/imps/FailedDeleteManager.java +++ b/curator-framework/src/main/java/org/apache/curator/framework/imps/FailedDeleteManager.java @@ -26,6 +26,13 @@ class FailedDeleteManager { private final Logger log = LoggerFactory.getLogger(getClass()); private final CuratorFramework client; + + volatile FailedDeleteManagerListener debugListener = null; + + interface FailedDeleteManagerListener + { + public void pathAddedForDelete(String path); + } FailedDeleteManager(CuratorFramework client) { @@ -34,6 +41,12 @@ class FailedDeleteManager void addFailedDelete(String path) { + if ( debugListener != null ) + { + debugListener.pathAddedForDelete(path); + } + + if ( client.isStarted() ) { log.debug("Path being added to guaranteed delete set: " + path); http://git-wip-us.apache.org/repos/asf/curator/blob/b174dfb1/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFailedDeleteManager.java ---------------------------------------------------------------------- diff --git a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFailedDeleteManager.java b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFailedDeleteManager.java index b684276..6599745 100644 --- a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFailedDeleteManager.java +++ b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFailedDeleteManager.java @@ -20,6 +20,9 @@ package org.apache.curator.framework.imps; import org.apache.curator.framework.CuratorFramework; import org.apache.curator.framework.CuratorFrameworkFactory; +import org.apache.curator.framework.api.BackgroundCallback; +import org.apache.curator.framework.api.CuratorEvent; +import org.apache.curator.framework.imps.FailedDeleteManager.FailedDeleteManagerListener; import org.apache.curator.framework.state.ConnectionState; import org.apache.curator.framework.state.ConnectionStateListener; import org.apache.curator.retry.ExponentialBackoffRetry; @@ -28,10 +31,13 @@ import org.apache.curator.test.BaseClassForTests; import org.apache.curator.test.Timing; import org.apache.curator.utils.CloseableUtils; import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.KeeperException.NoNodeException; import org.testng.Assert; import org.testng.annotations.Test; + import java.util.concurrent.CountDownLatch; import java.util.concurrent.Semaphore; +import java.util.concurrent.atomic.AtomicBoolean; public class TestFailedDeleteManager extends BaseClassForTests { @@ -276,4 +282,84 @@ public class TestFailedDeleteManager extends BaseClassForTests CloseableUtils.closeQuietly(client); } } + + @Test + public void testGuaranteedDeleteOnNonExistentNodeInForeground() throws Exception + { + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1)); + client.start(); + + final AtomicBoolean pathAdded = new AtomicBoolean(false); + + ((CuratorFrameworkImpl)client).getFailedDeleteManager().debugListener = new FailedDeleteManagerListener() + { + + @Override + public void pathAddedForDelete(String path) + { + pathAdded.set(true); + } + }; + + try + { + client.delete().guaranteed().forPath("/nonexistent"); + Assert.fail(); + } + catch(NoNodeException e) + { + //Exception is expected, the delete should not be retried + Assert.assertFalse(pathAdded.get()); + } + finally + { + client.close(); + } + } + + @Test + public void testGuaranteedDeleteOnNonExistentNodeInBackground() throws Exception + { + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1)); + client.start(); + + final AtomicBoolean pathAdded = new AtomicBoolean(false); + + ((CuratorFrameworkImpl)client).getFailedDeleteManager().debugListener = new FailedDeleteManagerListener() + { + + @Override + public void pathAddedForDelete(String path) + { + pathAdded.set(true); + } + }; + + final CountDownLatch backgroundLatch = new CountDownLatch(1); + + BackgroundCallback background = new BackgroundCallback() + { + + @Override + public void processResult(CuratorFramework client, CuratorEvent event) + throws Exception + { + backgroundLatch.countDown(); + } + }; + + try + { + client.delete().guaranteed().inBackground(background).forPath("/nonexistent"); + + backgroundLatch.await(); + + //Exception is expected, the delete should not be retried + Assert.assertFalse(pathAdded.get()); + } + finally + { + client.close(); + } + } }
