diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java index a6f60539089..9a137c8ca09 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java @@ -446,21 +446,25 @@ public void createNode(final String path, byte[] data, List acl, long ephem throw new NoNodeException(); } synchronized (parent) { - // Add the ACL to ACL cache first, to avoid the ACL not being - // created race condition during fuzzy snapshot sync. - // - // This is the simplest fix, which may add ACL reference count - // again if it's already counted in the ACL map of fuzzy - // snapshot, which might also happen for deleteNode txn, but - // at least it won't cause the ACL not exist issue. - // - // Later we can audit and delete all non-referenced ACLs from - // ACL map when loading the snapshot/txns from disk, like what - // we did for the global sessions. - Long acls = aclCache.convertAcls(acl); - Set children = parent.getChildren(); if (children.contains(childName)) { + DataNode child = nodes.get(path); + if (child == null) { + LOG.error("Parent claims child exists but it does not: " + path); + } else { + synchronized (child) { + // Due to fuzzy snapshot, it's possible that a node + // exists but its ACL is missing in the cache. This is + // because they serialized ACL cache before node is + // created but serialized nodes after creation. In this + // case, we'll replay node creation transaction. We + // should add it to cache and reset the ACL if the + // previous longval does not exist. + if (!aclCache.isLongCached(child.acl)) { + child.acl = aclCache.convertAcls(acl); + } + } + } throw new NodeExistsException(); } @@ -478,6 +482,7 @@ public void createNode(final String path, byte[] data, List acl, long ephem parent.stat.setCversion(parentCVersion); parent.stat.setPzxid(zxid); } + Long acls = aclCache.convertAcls(acl); DataNode child = new DataNode(data, acls, stat); parent.addChild(childName); nodes.postChange(parentName, parent); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java index 91b9ca5dfc6..7ff3cfef018 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java @@ -95,6 +95,19 @@ public synchronized List convertLong(Long longVal) { return acls; } + /** + * checks if longval is cached (convertLong can convert it to ACLs). + * + * @param longVal + * @return true if it's cached. + */ + public synchronized boolean isLongCached(long longVal) { + if (longVal == OPEN_UNSAFE_ACL_ID) { + return true; + } + return longKeyMap.containsKey(longVal); + } + private long incrementIndex() { return ++aclIndex; } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java index 466cf3f4b07..ddf28aa5d21 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java @@ -45,6 +45,7 @@ import org.apache.zookeeper.test.ClientBase; import org.apache.zookeeper.test.TestUtils; import org.apache.zookeeper.txn.CreateTxn; +import org.apache.zookeeper.txn.DeleteTxn; import org.apache.zookeeper.txn.SetDataTxn; import org.apache.zookeeper.txn.TxnDigest; import org.apache.zookeeper.txn.TxnHeader; @@ -333,6 +334,14 @@ public void testDirCheckWithLogFilesInSnapDir() throws IOException { @Test public void testACLCreatedDuringFuzzySnapshotSync() throws IOException { DataTree leaderDataTree = new DataTree(); + // Populate the initial ACL cache with some entries before snapshotting. + TxnHeader hdr1 = new TxnHeader(1, 2, 2, 2, ZooDefs.OpCode.create); + Record txn1 = new CreateTxn("/a1", "foo".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, false, -1); + leaderDataTree.processTxn(hdr1, txn1); + + TxnHeader hdr2 = new TxnHeader(1, 2, 3, 2, ZooDefs.OpCode.delete); + Record txn2 = new DeleteTxn("/a1"); + leaderDataTree.processTxn(hdr2, txn2); // Start the simulated snap-sync by serializing ACL cache. File file = File.createTempFile("snapshot", "zk"); @@ -341,9 +350,9 @@ public void testACLCreatedDuringFuzzySnapshotSync() throws IOException { leaderDataTree.serializeAcls(oa); // Add couple of transaction in-between. - TxnHeader hdr1 = new TxnHeader(1, 2, 2, 2, ZooDefs.OpCode.create); - Record txn1 = new CreateTxn("/a1", "foo".getBytes(), ZooDefs.Ids.CREATOR_ALL_ACL, false, -1); - leaderDataTree.processTxn(hdr1, txn1); + TxnHeader hdr3 = new TxnHeader(1, 2, 4, 2, ZooDefs.OpCode.create); + Record txn3 = new CreateTxn("/a1", "foo".getBytes(), ZooDefs.Ids.CREATOR_ALL_ACL, false, -1); + leaderDataTree.processTxn(hdr3, txn3); // Finish the snapshot. leaderDataTree.serializeNodes(oa); @@ -354,13 +363,16 @@ public void testACLCreatedDuringFuzzySnapshotSync() throws IOException { InputArchive ia = BinaryInputArchive.getArchive(is); DataTree followerDataTree = new DataTree(); followerDataTree.deserialize(ia, "tree"); - followerDataTree.processTxn(hdr1, txn1); + is.close(); + followerDataTree.processTxn(hdr3, txn3); - DataNode a1 = leaderDataTree.getNode("/a1"); - assertNotNull(a1); - assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, leaderDataTree.getACL(a1)); + DataNode a1FromLeader = leaderDataTree.getNode("/a1"); + assertNotNull(a1FromLeader); + assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, leaderDataTree.getACL(a1FromLeader)); - assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, followerDataTree.getACL(a1)); + DataNode a1FromFollower = followerDataTree.getNode("/a1"); + assertNotNull(a1FromFollower); + assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, followerDataTree.getACL(a1FromFollower)); } @Test