ZOOKEEPER-1046: Creating a new sequential node results in a ZNODEEXISTS error

git-svn-id: https://svn.apache.org/repos/asf/zookeeper/trunk@1146025 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -242,6 +242,8 @@ BUGFIXES:
   ZOOKEEPER-1046. Creating a new sequential node results in a ZNODEEXISTS error. (breed via camille)
 
   ZOOKEEPER-1097. Quota is not correctly rehydrated on snapshot reload (camille via henryr)
+  
+  ZOOKEEPER-1046. Small fix: Creating a new sequential node results in a ZNODEEXISTS error. (Vishal K via camille)
 
 IMPROVEMENTS:
   ZOOKEEPER-724. Improve junit test integration - log harness information 

src/java/main/org/apache/zookeeper/server/DataTree.java

@@ -1299,20 +1299,22 @@ public class DataTree {
     }
 
      /**
-      * If the znode for the specified path is found, then this method
-      * increments the cversion and sets its pzxid to the zxid passed
-      * in the second argument. A NoNodeException is thrown if the znode is
-      * not found.
+      * This method sets the Cversion and Pzxid for the specified node to the
+      * values passed as arguments. The values are modified only if newCversion
+      * is greater than the current Cversion. A NoNodeException is thrown if
+      * a znode for the specified path is not found.
       *
       * @param path
-      *     Full path to the znode whose cversion needs to be incremented.
+      *     Full path to the znode whose Cversion needs to be modified.
       *     A "/" at the end of the path is ignored.
+      * @param newCversion
+      *     Value to be assigned to Cversion
       * @param zxid
-      *     Value to be assigned to pzxid
+      *     Value to be assigned to Pzxid
       * @throws KeeperException.NoNodeException
       *     If znode not found.
       **/
-    public void incrementCversion(String path, long zxid)
+    public void setCversionPzxid(String path, int newCversion, long zxid)
         throws KeeperException.NoNodeException {
         if (path.endsWith("/")) {
            path = path.substring(0, path.length() - 1);
@@ -1322,8 +1324,13 @@ public class DataTree {
             throw new KeeperException.NoNodeException(path);
         }
         synchronized (node) {
-            node.stat.setCversion(node.stat.getCversion() + 1);
-            node.stat.setPzxid(zxid);
+            if(newCversion == -1) {
+                newCversion = node.stat.getCversion() + 1;
+            }
+            if (newCversion > node.stat.getCversion()) {
+                node.stat.setCversion(newCversion);
+                node.stat.setPzxid(zxid);
+            }
         }
     }
 }

src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java

@@ -25,21 +25,19 @@ import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.jute.Record;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.KeeperException.Code;
 import org.apache.zookeeper.ZooDefs.OpCode;
 import org.apache.zookeeper.server.DataTree;
+import org.apache.zookeeper.server.DataTree.ProcessTxnResult;
 import org.apache.zookeeper.server.Request;
 import org.apache.zookeeper.server.ZooTrace;
 import org.apache.zookeeper.server.persistence.TxnLog.TxnIterator;
 import org.apache.zookeeper.txn.CreateSessionTxn;
 import org.apache.zookeeper.txn.CreateTxn;
 import org.apache.zookeeper.txn.TxnHeader;
-import org.apache.zookeeper.KeeperException.Code;
-import org.apache.zookeeper.KeeperException.NoNodeException;
-import org.apache.zookeeper.server.DataTree.ProcessTxnResult;
-import org.apache.zookeeper.KeeperException.NoNodeException;
-import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * This is a helper class 
@@ -201,10 +199,10 @@ public class FileTxnSnapLog {
 
         /**
          * Snapshots are taken lazily. It can happen that the child
-         * znodes of a parent are modified (deleted or created) after the parent
+         * znodes of a parent are created after the parent
          * is serialized. Therefore, while replaying logs during restore, a
-         * delete/create might fail because the node was already
-         * deleted/created.
+         * create might fail because the node was already
+         * created.
          *
          * After seeing this failure, we should increment
          * the cversion of the parent znode since the parent was serialized
@@ -213,17 +211,18 @@ public class FileTxnSnapLog {
          * Note, such failures on DT should be seen only during
          * restore.
          */
-        if ((hdr.getType() == OpCode.create &&
-                rc.err == Code.NODEEXISTS.intValue()) &&
-                ((CreateTxn)txn).getParentCVersion() == -1) {
-            LOG.debug("Failed Txn: " + hdr.getType() + " path:" +
-                  rc.path + " err: " + rc.err);
+        if (hdr.getType() == OpCode.create &&
+                rc.err == Code.NODEEXISTS.intValue()) {
+            LOG.debug("Adjusting parent cversion for Txn: " + hdr.getType() +
+                    " path:" + rc.path + " err: " + rc.err);
             int lastSlash = rc.path.lastIndexOf('/');
             String parentName = rc.path.substring(0, lastSlash);
+            CreateTxn cTxn = (CreateTxn)txn;
             try {
-                dt.incrementCversion(parentName, hdr.getZxid());
+                dt.setCversionPzxid(parentName, cTxn.getParentCVersion(),
+                        hdr.getZxid());
             } catch (KeeperException.NoNodeException e) {
-                LOG.error("Failed to increment parent cversion for: " +
+                LOG.error("Failed to set parent cversion for: " +
                       parentName, e);
                 throw e;
             }

src/java/test/org/apache/zookeeper/test/DataTreeTest.java

@@ -70,10 +70,10 @@ public class DataTreeTest extends ZKTestCase {
     public void testIncrementCversion() throws Exception {
         dt.createNode("/test", new byte[0], null, 0, dt.getNode("/").stat.getCversion()+1, 1, 1);
         DataNode zk = dt.getNode("/test");
-        long prevCversion = zk.stat.getCversion();
+        int prevCversion = zk.stat.getCversion();
         long prevPzxid = zk.stat.getPzxid();
-        dt.incrementCversion("/test/",  prevPzxid + 1);
-        long newCversion = zk.stat.getCversion();
+        dt.setCversionPzxid("/test/",  prevCversion + 1, prevPzxid + 1);
+        int newCversion = zk.stat.getCversion();
         long newPzxid = zk.stat.getPzxid();
         Assert.assertTrue("<cversion, pzxid> verification failed. Expected: <" +
                 (prevCversion + 1) + ", " + (prevPzxid + 1) + ">, found: <" +

src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java

@@ -136,7 +136,11 @@ public class LoadFromLogTest extends ZKTestCase implements  Watcher {
 
         // Make create to fail, then verify cversion.
         LOG.info("Attempting to create " + "/test/" + (count - 1));
-        doOp(logFile, OpCode.create, "/test/" + (count - 1), dt, zk);
+        doOp(logFile, OpCode.create, "/test/" + (count - 1), dt, zk, -1);
+
+        LOG.info("Attempting to create " + "/test/" + (count - 1));
+        doOp(logFile, OpCode.create, "/test/" + (count - 1), dt, zk,
+                zk.stat.getCversion() + 1);
 
         // Make delete fo fail, then verify cversion.
         // this doesn't happen anymore, we only set the cversion on create
@@ -148,7 +152,7 @@ public class LoadFromLogTest extends ZKTestCase implements  Watcher {
      * if cversion before the operation is 1 less than cversion afer.
      */
     private void doOp(FileTxnSnapLog logFile, int type, String path,
-            DataTree dt, DataNode parent) throws Exception {
+            DataTree dt, DataNode parent, int cversion) throws Exception {
         int lastSlash = path.lastIndexOf('/');
         String parentName = path.substring(0, lastSlash);
 
@@ -171,11 +175,11 @@ public class LoadFromLogTest extends ZKTestCase implements  Watcher {
         } else if (type == OpCode.create) {
             txnHeader = new TxnHeader(0xabcd, 0x123, prevPzxid + 1,
                     System.currentTimeMillis(), OpCode.create);
-            txn = new CreateTxn(path, new byte[0], null, false, -1);
+            txn = new CreateTxn(path, new byte[0], null, false, cversion);
         }
         logFile.processTransaction(txnHeader, dt, null, txn);
 
-        long newCversion = parent.stat.getCversion();
+        int newCversion = parent.stat.getCversion();
         long newPzxid = parent.stat.getPzxid();
         child = dt.getChildren(parentName, null, null);
         childStr = "";
@@ -206,8 +210,8 @@ public class LoadFromLogTest extends ZKTestCase implements  Watcher {
         BinaryInputArchive ia  = BinaryInputArchive.getArchive(in);
         FileHeader header = new FileHeader();
         header.deserialize(ia, "fileheader");
-        LOG.info("Expected header :" + header.getMagic() +
-              " Received : " + FileTxnLog.TXNLOG_MAGIC);
+        LOG.info("Received magic : " + header.getMagic() +
+              " Expected : " + FileTxnLog.TXNLOG_MAGIC);
         Assert.assertTrue("Missing magic number ",
               header.getMagic() == FileTxnLog.TXNLOG_MAGIC);
     }