Repository navigation
Fix paginated getChildren returning no children for the root path - #160
Conversation
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>
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>
sjainit
left a comment
sjainit
left a comment
There was a problem hiding this comment.
Approving.
The one-line fix in DataTree.buildChildrenPathWithStat (use /child rather than //child for the root key) is correct and correctly scoped:
- Only the sorted paginated path was affected; the fast path returns
n.getChildren()directly, and non-root/chrooted listings were never broken.buildChildrenPathWithStatis reached only fromgetSortedPaginatedChildren, so this single change covers every affected case. - No wire/client change — only the internal lookup key changes; returned values are still relative child names in both paths.
Tests are genuinely discriminating: children are created in reverse lexical order with ascending czxid (so a name-sort regression would fail), and the per-page watch-count assertion catches the exact bug (pre-fix, the empty child list made page 1 look like the last page and registered the watch early). Confirmed fail-without-fix.
One rollout note (already documented in the description): keep #159's client-side rejection of unchrooted / in place until servers are fully upgraded, since a fixed client paging across a reconnect to an unfixed server (minCzxid > 0) would otherwise see a silently truncated / listing.
Non-blocking follow-up: getCounts shares the same path + "/" + child concat and would mis-count if a quota were ever set directly on / (fail-safe undercount via the node == null guard). Out of scope here; worth a separate issue at most.
|
Agreed — It is a different code path from the fix here, so I would rather handle it separately than widen this PR. |
Description
Fix the paginated
getChildren(opcode 71, used bygetAllChildrenPaginated,getChildrenIteratorand the bounded-pagegetChildrenoverload) returning no children for the root path/.Why:
DataTree.buildChildrenPathWithStatlooks each child up aspath + "/" + child. For/that produces//child, which is never in the node map, so every child is skipped and the server replies with an empty last page. The client trusts that reply, so the listing of/silently looks empty.Only paginated listings that take the sorted path are affected: a bounded page size,
minCzxId > 0, or a response larger thanjute.maxbuffer. A small, unbounded listing of/uses the fast path and was never affected. Non-root paths and chrooted clients (the chroot is prepended, so the server sees/chroot) were never affected. PlaingetChildren/getChildren2do not use this code.How: use
/childas the lookup key when the path is/; every other path is unchanged. No wire-protocol or client change.Found while adding
deleteall -c(#159), which had to guard against this on the client.Watch timing on
/changes slightly. Before this fix the sorted path on/built an empty child list, soupdateNextPagesawlastAddedIndex == size - 1and marked the very first page as the last one, registering the child watch immediately. With the fix,/behaves like every other path: the server registers the watch only on the genuinely last page (or on the fast path). A caller that reads only the first page of/and never paginates to the end no longer gets that accidental early watch.Rollout. Server-side only, no client or wire change, and no persistence, snapshot, txn-log or digest impact. The paginated API is reachable only from the LinkedIn Java client, where the two callers are zkCli (which ships inside the server package, so it is always the same build) and Helix (pagination is on by default, but any root that fits in
jute.maxbuffertakes the fast path, so it is unaffected in practice).zk shell, the healthchecker and the C/C++/Python clients use kazoo or stock Apache and have no paginated API at all. One caveat while a rollout is in flight: a client that pages across a reconnect from a fixed to an unfixed server sendsminCzxid > 0, and the unfixed server answers with an empty page marked as the last one, which truncates the listing silently.#159blocks the only caller that would do this on/.origin/li-dev/base-3.8.4carries the same bug and will need the same one-line change if that branch is picked up.Tests
The following tests are written for this issue:
DataTreeTesttestGetChildrenPaginatedOnRoot(bounded pages over/return every child, includingzookeeper, in creation order; the watch is asserted after each page so that it is absent before the last page and present on it;minCzxId > 0on/returns the expected children and reports the last page)GetChildrenPaginatedTesttestPaginationIteratorOnRoot(end-to-endgetChildrenIterator("/")returns the children with no duplicates and in creation order)Both new tests were confirmed to fail with the fix reverted and to pass with it:
The children are deliberately created in reverse lexical order, so a regression that sorted by name instead of czxid would also fail.
The following is the result of the
mvn testcommand on the appropriate module (zookeeper-server, JDK 11):Also passing:
-Pfull-build apache-rat:check verify -DskipTests spotbugs:check checkstyle:checkonzookeeper-server.Self-reviewed by two independent model reviews (Claude Opus 5.5 and GPT-6.1 Sol), each asked to re-derive every claim from source. Neither found a defect in the one-line fix, and both independently flagged that the original watch assertion would have passed without the fix — it only checked the count after pagination finished, and with the bug the empty child list makes the first page look like the last one. That is corrected in the second commit. These ran outside the branch-review receipt flow, so the box is left unchecked.