Skip to content

Commit 1d5ee7c

Browse files
committed
ZOOKEEPER-5097: Make DataTree snapshot serialization iterative
Reviewers: PDavid, kezhuw Author: anmolnar Closes #2476 from anmolnar/ZOOKEEPER-5097
1 parent 4be9222 commit 1d5ee7c

2 files changed

Lines changed: 82 additions & 26 deletions

File tree

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

Lines changed: 41 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,11 @@
2222
import java.io.IOException;
2323
import java.io.PrintWriter;
2424
import java.nio.ByteBuffer;
25+
import java.util.ArrayDeque;
2526
import java.util.ArrayList;
2627
import java.util.Collection;
2728
import java.util.Collections;
29+
import java.util.Deque;
2830
import java.util.HashMap;
2931
import java.util.HashSet;
3032
import java.util.LinkedList;
@@ -1340,32 +1342,45 @@ private void setupQuota() {
13401342
* @throws IOException
13411343
*/
13421344
void serializeNode(OutputArchive oa, StringBuilder path) throws IOException {
1343-
String pathString = path.toString();
1344-
DataNode node = getNode(pathString);
1345-
if (node == null) {
1346-
return;
1347-
}
1348-
String[] children = null;
1349-
DataNode nodeCopy;
1350-
synchronized (node) {
1351-
StatPersisted statCopy = new StatPersisted();
1352-
copyStatPersisted(node.stat, statCopy);
1353-
//we do not need to make a copy of node.data because the contents
1354-
//are never changed
1355-
nodeCopy = new DataNode(node.data, node.acl, statCopy);
1356-
Set<String> childs = node.getChildren();
1357-
children = childs.toArray(new String[childs.size()]);
1358-
}
1359-
serializeNodeData(oa, pathString, nodeCopy);
1360-
path.append('/');
1361-
int off = path.length();
1362-
for (String child : children) {
1363-
// since this is single buffer being resused
1364-
// we need
1365-
// to truncate the previous bytes of string.
1366-
path.delete(off, Integer.MAX_VALUE);
1367-
path.append(child);
1368-
serializeNode(oa, path);
1345+
// Iterative, explicit-stack pre-order traversal of the subtree rooted
1346+
// at `path`. This was previously written as recursion with one frame
1347+
// per znode level, which does not scale to very deep trees; walking the
1348+
// tree with a heap-allocated stack removes the dependency on tree depth.
1349+
//
1350+
// The traversal order (pre-order, children in the same order as
1351+
// before), the per-node locking, and the emitted records are unchanged,
1352+
// so the produced snapshot bytes are identical and the on-disk format is
1353+
// unchanged. The explicit stack holds at most the unvisited frontier of
1354+
// the tree (a single entry for a chain), all of which the in-memory
1355+
// DataTree already holds.
1356+
Deque<String> pathStack = new ArrayDeque<>();
1357+
pathStack.push(path.toString());
1358+
while (!pathStack.isEmpty()) {
1359+
String pathString = pathStack.pop();
1360+
DataNode node = getNode(pathString);
1361+
if (node == null) {
1362+
continue;
1363+
}
1364+
String[] children = null;
1365+
DataNode nodeCopy;
1366+
synchronized (node) {
1367+
StatPersisted statCopy = new StatPersisted();
1368+
copyStatPersisted(node.stat, statCopy);
1369+
//we do not need to make a copy of node.data because the contents
1370+
//are never changed
1371+
nodeCopy = new DataNode(node.data, node.acl, statCopy);
1372+
Set<String> childs = node.getChildren();
1373+
children = childs.toArray(new String[childs.size()]);
1374+
}
1375+
// Called outside the node lock: writeRecord can block and holding
1376+
// the lock would stall writers (ZOOKEEPER-2201).
1377+
serializeNodeData(oa, pathString, nodeCopy);
1378+
// Push children in reverse so that, popped from this LIFO stack,
1379+
// they are visited in the same order as the previous recursion.
1380+
String childPathPrefix = pathString + "/";
1381+
for (int i = children.length - 1; i >= 0; i--) {
1382+
pathStack.push(childPathPrefix + children[i]);
1383+
}
13691384
}
13701385
}
13711386

‎zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,47 @@ public void run() {
418418
assertTrue(ranTestCase.get(), "Didn't find the expected node");
419419
}
420420

421+
/**
422+
* Snapshot serialization must not depend on tree depth. This builds a very
423+
* deep znode chain and serializes it on a thread with a small (512 KiB)
424+
* stack, so the test is independent of the runner's default -Xss, then
425+
* checks the result round-trips back to an equivalent tree.
426+
*/
427+
@Test
428+
@Timeout(value = 120)
429+
public void testSerializeDeepTree() throws Exception {
430+
final int depth = 4000;
431+
final DataTree tree = new DataTree();
432+
final StringBuilder path = new StringBuilder();
433+
for (int i = 0; i < depth; i++) {
434+
path.append("/a");
435+
tree.createNode(path.toString(), new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, -1L, i + 1, i + 1, 1);
436+
}
437+
438+
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
439+
final BinaryOutputArchive oa = BinaryOutputArchive.getArchive(baos);
440+
final Throwable[] failure = new Throwable[1];
441+
final Thread serializer = new Thread(null, () -> {
442+
try {
443+
tree.serialize(oa, "tree");
444+
} catch (Throwable t) {
445+
failure[0] = t;
446+
}
447+
}, "deep-tree-serializer", 512 * 1024L);
448+
serializer.start();
449+
serializer.join();
450+
451+
assertNull(failure[0], "serialize() must not fail on a deep tree");
452+
453+
// The produced bytes must load back into an equivalent tree.
454+
final DataTree restored = new DataTree();
455+
final BinaryInputArchive ia =
456+
BinaryInputArchive.getArchive(new ByteArrayInputStream(baos.toByteArray()));
457+
restored.deserialize(ia, "tree");
458+
assertNotNull(restored.getNode(path.toString()),
459+
"the deepest node must survive a serialize/deserialize round-trip");
460+
}
461+
421462
@Test
422463
@Timeout(value = 60)
423464
public void testReconfigACLClearOnDeserialize() throws Exception {

0 commit comments

Comments
 (0)