Skip to content

Commit d7d56cf

Browse files
laxman-chCopilot
andauthored
Fix paginated getChildren returning no children for the root path (#160)
* Fix paginated getChildren returning no children for the root path DataTree.buildChildrenPathWithStat looked children up as path + "/" + child, which yields "//child" for "/", so every child was skipped and the server replied with an empty last page. Use "/child" for the root. Only paginated listings that take the sorted path were affected: bounded page sizes, minCzxId > 0, or a response larger than jute.maxbuffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Strengthen root pagination tests The previous assertions could pass even without the DataTree fix: - The watch count was only checked after pagination finished. With the "//child" bug the child list is empty, so index ends at -1 == size-1 and the watch is registered on the very first page -- yielding the same final count. The count is now asserted after every page: unchanged before the last page, incremented on it. - Children were created in lexical order, which matched czxid order, so a regression that sorted by name instead of czxid would still have passed. They are now created in reverse lexical order. - "zookeeper" was filtered out, so its presence and position were never checked. The full ordered list is now asserted. - The end-to-end test collected into a Set, hiding duplicates and ordering errors. It now collects into a List and asserts creation order. Both tests were confirmed to fail with the fix reverted and pass with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 9b0eeef commit d7d56cf

3 files changed

Lines changed: 88 additions & 1 deletion

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -976,8 +976,10 @@ private void buildChildrenPathWithStat(DataNode n, String path, Stat stat, long
976976
if (stat != null) {
977977
n.copyStat(stat);
978978
}
979+
// The root's children are keyed as "/child", not "//child"
980+
final String prefix = "/".equals(path) ? path : path + "/";
979981
for (String child : n.getChildren()) {
980-
DataNode childNode = nodes.get(path + "/" + child);
982+
DataNode childNode = nodes.get(prefix + child);
981983
if (null != childNode) {
982984
final long czxId = childNode.stat.getCzxid();
983985

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import java.io.StringWriter;
3434
import java.lang.reflect.Field;
3535
import java.util.ArrayList;
36+
import java.util.Arrays;
3637
import java.util.List;
3738
import java.util.Map;
3839
import java.util.concurrent.CompletableFuture;
@@ -415,6 +416,57 @@ public void testGetChildrenPaginated() throws NodeExistsException, NoNodeExcepti
415416
assertEquals(0, nextPage.getMinCzxidOffset());
416417
}
417418

419+
@Test(timeout = 60000)
420+
public void testGetChildrenPaginatedOnRoot() throws NodeExistsException, NoNodeException {
421+
final int firstCzxId = 1000;
422+
final int countNodes = 10;
423+
424+
DataTree dt = new DataTree();
425+
// Create the children in reverse lexical order so that ordering by czxid (correct) and
426+
// ordering by name (incorrect) produce different results, and the test can tell them apart.
427+
List<String> childrenCreated = new ArrayList<>(countNodes);
428+
for (int i = 0; i < countNodes; ++i) {
429+
final String name = "test-" + (countNodes - 1 - i);
430+
dt.createNode("/" + name, new byte[0], null, 0, dt.getNode("/").stat.getCversion() + 1, firstCzxId + i, 1);
431+
childrenCreated.add(name);
432+
}
433+
434+
// "/zookeeper" has czxid 0, so it sorts ahead of everything created above.
435+
List<String> expected = new ArrayList<>();
436+
expected.add("zookeeper");
437+
expected.addAll(childrenCreated);
438+
439+
// Bounded pages on "/" must return every child, in creation order
440+
List<String> collected = new ArrayList<>();
441+
PaginationNextPage nextPage = new PaginationNextPage();
442+
long minCzxid = -1;
443+
int offset = 0;
444+
final int watchCount = dt.getWatchCount();
445+
boolean reachedLastPage = false;
446+
for (int i = 0; i < countNodes + 2; ++i) {
447+
List<String> page = dt.getPaginatedChildren("/", null, new DummyWatcher(), 3, minCzxid, offset, nextPage);
448+
collected.addAll(page);
449+
if (nextPage.getMinCzxid() == ZooDefs.GetChildrenPaginated.lastPageMinCzxid) {
450+
reachedLastPage = true;
451+
assertEquals("The watch must be set on the last page", watchCount + 1, dt.getWatchCount());
452+
break;
453+
}
454+
// Asserting this on every intermediate page is what catches the "//child" bug: with the
455+
// bug the very first page looks like a last page and registers the watch immediately.
456+
assertEquals("The watch must not be set before the last page", watchCount, dt.getWatchCount());
457+
minCzxid = nextPage.getMinCzxid();
458+
offset = nextPage.getMinCzxidOffset();
459+
}
460+
assertTrue("Pagination never reported a last page", reachedLastPage);
461+
assertEquals(expected, collected);
462+
463+
// minCzxId > 0 skips the fast path even with an unbounded page size
464+
List<String> result = dt.getPaginatedChildren("/", null, null, Integer.MAX_VALUE, firstCzxId + countNodes - 2, 0, nextPage);
465+
assertEquals(Arrays.asList("test-1", "test-0"), result);
466+
assertEquals("The remainder fits in one page, so it is the last page",
467+
ZooDefs.GetChildrenPaginated.lastPageMinCzxid, nextPage.getMinCzxid());
468+
}
469+
418470
@Test(timeout = 60000)
419471
public void testGetChildrenPaginatedWithOffset() throws NodeExistsException, NoNodeException {
420472
final String rootPath = "/children";

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
package org.apache.zookeeper.test;
2020

2121
import static org.junit.Assert.fail;
22+
import java.util.ArrayList;
2223
import java.util.HashMap;
2324
import java.util.HashSet;
2425
import java.util.List;
@@ -127,6 +128,38 @@ public void testPaginationIterator() throws Exception {
127128
Assert.assertEquals(createdChildrenMetadata.keySet(), readChildrenMetadata);
128129
}
129130

131+
@Test(timeout = 30000)
132+
public void testPaginationIteratorOnRoot() throws Exception {
133+
final String prefix = "/testPaginationRoot-" + UUID.randomUUID();
134+
List<String> expected = new ArrayList<String>();
135+
for (int i = 0; i < 10; i++) {
136+
zk.create(prefix + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
137+
expected.add(prefix.substring(1) + i);
138+
}
139+
140+
// Collect into a List (not a Set) so duplicated or out-of-order pages are caught.
141+
List<String> actual = new ArrayList<String>();
142+
RemoteIterator<String> childrenIterator = zk.getChildrenIterator("/", null, 3, -1);
143+
while (childrenIterator.hasNext()) {
144+
actual.add(childrenIterator.next());
145+
}
146+
147+
Assert.assertEquals("Root pagination returned duplicates: " + actual,
148+
new HashSet<String>(actual).size(), actual.size());
149+
Assert.assertTrue("Missing children of root: " + expected + " vs " + actual, actual.containsAll(expected));
150+
Assert.assertTrue("Missing /zookeeper: " + actual, actual.contains("zookeeper"));
151+
152+
// The nodes were created in order, so they must come back in that order. Other tests may
153+
// leave unrelated nodes at the root, so compare only the ones this test created.
154+
List<String> actualOurs = new ArrayList<String>();
155+
for (String child : actual) {
156+
if (child.startsWith(prefix.substring(1))) {
157+
actualOurs.add(child);
158+
}
159+
}
160+
Assert.assertEquals("Root children returned out of creation order", expected, actualOurs);
161+
}
162+
130163
/*
131164
* This test validates a known list of children is returned by the iterator despite server failures.
132165
* After the iterator is created successfully, the following logic drives the rest of the test:

0 commit comments

Comments
 (0)