This is an automated email from the ASF dual-hosted git repository.
andor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push:
new f4c7b69 ZOOKEEPER-3531: Synchronization on ACLCache cause cluster to
hang whe…
f4c7b69 is described below
commit f4c7b698bd239bcd15ee380d2ee38814dba432cd
Author: Mcfatealan <[email protected]>
AuthorDate: Wed Oct 9 16:38:44 2019 +0200
ZOOKEEPER-3531: Synchronization on ACLCache cause cluster to hang whe…
…n network/disk issues happen during datatree serialization
Author: Mcfatealan <[email protected]>
Reviewers: [email protected], [email protected]
Closes #1077 from mcfatealan/ZOOKEEPER-3531 and squashes the following
commits:
60109185e [Mcfatealan] minor
951dd5cac [Mcfatealan] fix checkstyle issue
96ea4a066 [Mcfatealan] finer-synchronization for deserialize
b79446a6f [Mcfatealan] minor fix for checkstyle
d5f7d04ab [Mcfatealan] ZOOKEEPER-3531: Synchronization on ACLCache cause
cluster to hang when network/disk issues happen during datatree serialization
---
.../zookeeper/server/ReferenceCountedACLCache.java | 40 +++++---
.../org/apache/zookeeper/server/DataTreeTest.java | 101 +++++++++++++++++++++
2 files changed, 129 insertions(+), 12 deletions(-)
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 a278abf..b92ee18 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
@@ -22,9 +22,9 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.concurrent.atomic.AtomicLong;
import org.apache.jute.Index;
import org.apache.jute.InputArchive;
@@ -99,14 +99,14 @@ public class ReferenceCountedACLCache {
return ++aclIndex;
}
- public synchronized void deserialize(InputArchive ia) throws IOException {
+ public void deserialize(InputArchive ia) throws IOException {
clear();
int i = ia.readInt("map");
+
+ LinkedHashMap<Long, List<ACL>> deserializedMap = new LinkedHashMap<>();
+ // keep read operations out of synchronization block
while (i > 0) {
Long val = ia.readLong("long");
- if (aclIndex < val) {
- aclIndex = val;
- }
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
if (j == null) {
@@ -118,17 +118,33 @@ public class ReferenceCountedACLCache {
aclList.add(acl);
j.incr();
}
- longKeyMap.put(val, aclList);
- aclKeyMap.put(aclList, val);
- referenceCounter.put(val, new AtomicLongWithEquals(0));
+
+ deserializedMap.put(val, aclList);
i--;
}
+
+ synchronized (this) {
+ for (Map.Entry<Long, List<ACL>> entry :
deserializedMap.entrySet()) {
+ Long val = entry.getKey();
+ List<ACL> aclList = entry.getValue();
+ if (aclIndex < val) {
+ aclIndex = val;
+ }
+
+ longKeyMap.put(val, aclList);
+ aclKeyMap.put(aclList, val);
+ referenceCounter.put(val, new AtomicLongWithEquals(0));
+ }
+ }
}
- public synchronized void serialize(OutputArchive oa) throws IOException {
- oa.writeInt(longKeyMap.size(), "map");
- Set<Map.Entry<Long, List<ACL>>> set = longKeyMap.entrySet();
- for (Map.Entry<Long, List<ACL>> val : set) {
+ public void serialize(OutputArchive oa) throws IOException {
+ Map<Long, List<ACL>> clonedLongKeyMap;
+ synchronized (this) {
+ clonedLongKeyMap = new HashMap<>(longKeyMap);
+ }
+ oa.writeInt(clonedLongKeyMap.size(), "map");
+ for (Map.Entry<Long, List<ACL>> val : clonedLongKeyMap.entrySet()) {
oa.writeLong(val.getKey(), "long");
List<ACL> aclList = val.getValue();
oa.startVector(aclList, "acls");
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
index b73e0b7..11eb836 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
+++
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java
@@ -24,6 +24,7 @@ import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
+import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;
import java.io.PrintWriter;
@@ -253,6 +254,106 @@ public class DataTreeTest extends ZKTestCase {
assertEquals("/bug is still in pTrie", "/",
pTrie.findMaxPrefix("/bug"));
}
+
+ /* ZOOKEEPER-3531 - org.apache.zookeeper.server.DataTree#serialize calls
the aclCache.serialize when doing
+ * dataree serialization, however,
org.apache.zookeeper.server.ReferenceCountedACLCache#serialize
+ * could get stuck at OutputArchieve.writeInt due to potential
network/disk issues.
+ * This can cause the system experiences hanging issues similar to
ZooKeeper-2201.
+ * This test verifies the fix that we should not hold ACL cache during
dumping aclcache to snapshots
+ */
+ @Test(timeout = 60000)
+ public void testSerializeDoesntLockACLCacheWhileWriting() throws Exception
{
+ DataTree tree = new DataTree();
+ tree.createNode("/marker", new byte[]{42}, null, -1, 1, 1, 1);
+ final AtomicBoolean ranTestCase = new AtomicBoolean();
+ DataOutputStream out = new DataOutputStream(new
ByteArrayOutputStream());
+ BinaryOutputArchive oa = new BinaryOutputArchive(out) {
+ @Override
+ public void writeInt(int size, String tag) throws IOException {
+ final Semaphore semaphore = new Semaphore(0);
+
+ new Thread(new Runnable() {
+ @Override
+ public void run() {
+
+ synchronized (tree.getReferenceCountedAclCache()) {
+ //When we lock ACLCache, allow writeRecord to
continue
+ semaphore.release();
+ }
+ }
+ }).start();
+
+ try {
+ boolean acquired = semaphore.tryAcquire(30,
TimeUnit.SECONDS);
+ //This is the real assertion - could another thread lock
+ //the ACLCache
+ assertTrue("Couldn't acquire a lock on the ACLCache while
we were calling tree.serialize", acquired);
+ } catch (InterruptedException e1) {
+ throw new RuntimeException(e1);
+ }
+ ranTestCase.set(true);
+
+ super.writeInt(size, tag);
+ }
+ };
+
+ tree.serialize(oa, "test");
+
+ //Let's make sure that we hit the code that ran the real assertion
above
+ assertTrue("Didn't find the expected node", ranTestCase.get());
+ }
+
+ /* ZOOKEEPER-3531 - similarly for aclCache.deserialize, we should not hold
lock either
+ */
+ @Test(timeout = 60000)
+ public void testDeserializeDoesntLockACLCacheWhileReading() throws
Exception {
+ DataTree tree = new DataTree();
+ tree.createNode("/marker", new byte[]{42}, null, -1, 1, 1, 1);
+ final AtomicBoolean ranTestCase = new AtomicBoolean();
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ DataOutputStream out = new DataOutputStream(baos);
+ BinaryOutputArchive oa = new BinaryOutputArchive(out);
+
+ tree.serialize(oa, "test");
+
+ DataTree tree2 = new DataTree();
+ DataInputStream in = new DataInputStream(new
ByteArrayInputStream(baos.toByteArray()));
+ BinaryInputArchive ia = new BinaryInputArchive(in) {
+ @Override
+ public long readLong(String tag) throws IOException {
+ final Semaphore semaphore = new Semaphore(0);
+
+ new Thread(new Runnable() {
+ @Override
+ public void run() {
+
+ synchronized (tree2.getReferenceCountedAclCache()) {
+ //When we lock ACLCache, allow readLong to continue
+ semaphore.release();
+ }
+ }
+ }).start();
+
+ try {
+ boolean acquired = semaphore.tryAcquire(30,
TimeUnit.SECONDS);
+ //This is the real assertion - could another thread lock
+ //the ACLCache
+ assertTrue("Couldn't acquire a lock on the ACLCache while
we were calling tree.deserialize", acquired);
+ } catch (InterruptedException e1) {
+ throw new RuntimeException(e1);
+ }
+ ranTestCase.set(true);
+
+ return super.readLong(tag);
+ }
+ };
+
+ tree2.deserialize(ia, "test");
+
+ //Let's make sure that we hit the code that ran the real assertion
above
+ assertTrue("Didn't find the expected node", ranTestCase.get());
+ }
+
/*
* ZOOKEEPER-2201 - OutputArchive.writeRecord can block for long periods of
* time, we must call it outside of the node lock.
