Repository navigation
Add deleteall -c to delete only the children of a znode - #159
Conversation
|
For an unchrooted connection, |
|
Thanks for catching this. Confirmed and fixed client-side in 12f3891:
The server-side |
|
Follow-up for the server-side root listing bug ( |
|
Confirmed — the subtree is fully materialized before the first delete, and One number that makes it concrete: zkCli defaults to The documented usage guideline is under 10k children per parent, so 500k–1M is well outside what the tool is likely to meet. Rather than restructure around Can look at streaming separately if you think it is worth doing. |
bellatrix007
left a comment
bellatrix007
left a comment
There was a problem hiding this comment.
Overall LGTM. The logic checks out (batch boundaries, chroot guard, numChildren verification), and the unchrooted-/ guard is well justified given #160 isn't deployed everywhere yet. The inline notes are mostly FYI or non-blocking.
Two of them (ZOOKEEPER-4325 and ZOOKEEPER-4710) are upstream fixes we'd pick up when moving to 3.8.x. 4325 is in 3.8.0+ and 4710 is in 3.8.4+ (JIRA lists only 3.9.0, but the 3.8.4 tag has the backport). Neither is in 3.6.x or 3.7.x releases.
Follow-up: #160 mentions li-dev/base-3.8.4 has the same root-listing bug. Worth tracking the port of both #159 and #160 there.
0888d45 to
d4133c1
Compare
|
Rebased onto current One correction on the follow-up: The earlier red Fixed in #169, now merged, which pins the runners to |
deleteall -c keeps the target znode and deletes only its descendants, listing the subtree with getAllChildrenPaginated so it works on znodes whose children list exceeds jute.maxbuffer. Existing deleteall/ZKUtil.deleteRecursive/ listSubTreeBFS behavior is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…docs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hild count Paginated listing of '/' on current servers can silently return no children, and the protected /zookeeper subtree can never be deleted, so 'deleteall / -c' either reported success without deleting anything or partially deleted data before failing. Reject it unless the client has a chroot, and replace the final emptiness check with the parent's numChildren so neither a concurrent create nor an incomplete listing is reported as success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…re message Two follow-ups from self-review: - The new listSubTreeBFS(ZooKeeper, String, boolean) overload was public. Called with paginated=true on the unchrooted root it returns just the root with no exception when the root children do not fit in a single page, because only deleteChildrenRecursive rejects that path. Making it package-private keeps the new public API surface unchanged and avoids shipping an API with a silent wrong-answer mode. All callers are in org.apache.zookeeper, so nothing else changes. - "Failed to delete some node(s) in the subtree!" is misleading for -c: the node itself is kept, so a false result usually means a writer created new children while the delete was running, not that a delete failed. The -c path now reports that children remain and names both possible causes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- "works on nodes whose children list does not fit in jute.maxbuffer" was overstated. Every page is requested with maxReturned=Integer.MAX_VALUE, so page size is bounded only by the server's jute.maxbuffer while the client independently rejects oversized incoming packets. If the server's limit is larger than the client's, the caller still gets "Packet len out of range". The client requirement is now stated explicitly. - The root rejection was justified partly by paginated listing of "/" being "not reliable on current servers", which stops being true once the server side is fixed. That wording invites someone to later remove the guard, which would be unsafe: the root can never be emptied because of the protected /zookeeper subtree. The permanent reason now leads, and the server bug is described as applying to servers without the root-pagination fix. - "neither a concurrent create nor an incomplete listing can be reported as success" was too absolute. exists() is a point-in-time read of the contacted server, not a lock, so a create racing the check can still be missed. Docs only; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… for deleteall -c Mirrors the javadoc correction in the CLI reference: pages are bounded by the server's jute.maxbuffer while the client enforces its own limit on the incoming packet, so the client must be configured with at least the server's value. Also notes that writers should be quiesced, since concurrent creates or deletes can abort the command or make it report remaining children. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
deleteChildrenRecursive lists the whole subtree into client memory before issuing the first delete, so the listing has to fit in the client's heap. zkCli defaults to -Xmx256m (ZK_CLIENT_HEAP in bin/zkEnv.sh). Log a warning when the target has more direct children than the threshold so an operator can raise the heap up front instead of discovering the limit as an OutOfMemoryError. The threshold sits an order of magnitude above the documented guideline of under 10,000 children per parent, so it only fires for nodes already well outside it. Behaviour is otherwise unchanged: one extra exists() call, no change to the traversal or the delete path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- deleteall -c now throws a CliException when children remain, so zkCli exits non-zero. ZooKeeperMain treats exec()'s return value as the watch flag, not success, and only sets a failing exit code when a CliException escapes, so the previous err.println left scripts unable to detect the failure. Plain deleteall is unchanged. Covered by a new CLI test. - Rename deleteInBatch's lastIndex to minIndex. The loop runs backwards, so the parameter is the lowest index visited and the old name read both ways. - Note the per-node round trip cost next to the client heap caveat. - Gate the concurrent-create test stub on the second exists() call. The pre-listing warning check made it fire twice, so the test passed only because the first child was drained by the listing and re-created. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d4133c1 to
735eb83
Compare
Description
Add a
-coption to thedeleteallCLI command that deletes all descendants of a znode but keeps the znode itself:deleteall <path> [-b batch size] [-c].Why: Clearing a parent without deleting it is a frequent operational need (for example, draining a Helix
MESSAGESznode that accumulated hundreds of thousands of children).deleteallalways removes the parent too, and it lists the subtree with a plaingetChildren, which fails withPacket len N is out of range!when the children list exceedsjute.maxbuffer.How:
ZKUtil.deleteChildrenRecursive(zk, path, batchSize): lists the subtree withgetAllChildrenPaginated, then deletes everything except the root, leaves first, using the existing batched/sync delete logic. A childless node is a no-op returningtrue. Because the root is kept, it checks the node'snumChildrenafter deleting and returnsfalseif any child is left (for example one created concurrently).-cis rejected (IllegalArgumentException, surfaced as a malformed-path error by the CLI) for/on a client without a chroot. The root can never be emptied, because its children include the protected/zookeepersubtree that the server refuses to delete, so this restriction is permanent and must not be relaxed. Separately, server-side paginated listing of/is broken on servers without the root-pagination fix (Fix paginated getChildren returning no children for the root path #160), which would make the attempt look like it succeeded on an empty tree. A chrooted/is supported.ZKUtil.listSubTreeBFS(zk, path, paginated)overload. The existing two-argument public method delegates withpaginated=false, so it still callsgetChildren. The overload is deliberately not public: withpaginated=trueon an unchrooted/it can return an incomplete subtree without raising, and onlydeleteChildrenRecursiveguards that case.deleteInBatchgets alastIndexparameter (0= include root,1= skip root). Existing callers pass0, which is identical to the previous behavior.DeleteAllCommand:-ccallsdeleteChildrenRecursive; without-cit callsdeleteRecursiveexactly as before. Afalseresult on the-cpath now reports that children remain rather than that a delete failed, since the node itself is kept and a concurrent create is the usual cause.zookeeperCLI.md.Operational notes
jute.maxbuffermust be at least the server's. Every page is requested withmaxReturned=Integer.MAX_VALUE, so the page size is bounded only by the server'sjute.maxbuffer, while the client independently rejects an oversized incoming packet. If the server's limit is the larger of the two, the caller still getsPacket len N is out of range!— the error this option exists to avoid. Run zkCli with a matching-Djute.maxbuffer.false. Re-running is safe and converges.Tests
The following tests are written for this issue:
ZKUtilTesttestDeleteChildrenRecursiveKeepsRootInAsyncModetestDeleteChildrenRecursiveKeepsRootInSyncModetestDeleteChildrenRecursiveOnChildlessRoottestDeleteChildrenRecursiveBeyondJuteMaxBuffertestDeleteChildrenRecursiveBatchBoundariestestDeleteChildrenRecursiveDoesNotTouchSiblingsOrParenttestDeleteChildrenRecursiveOnMissingNodetestDeleteChildrenRecursiveInvalidPathtestListSubTreeBFSPaginatedMatchesDefaulttestListSubTreeBFSPaginatedFromRootBuildsValidPathstestDeleteRecursiveStillDeletesRoottestDeleteChildrenRecursiveRejectsUnchrootedRoottestDeleteChildrenRecursiveOnChrootedRoottestDeleteChildrenRecursiveReportsChildCreatedConcurrentlyZooKeeperTesttestDeleteChildrenOnlyClitestDeleteChildrenOnlyCliOnMissingNodetestDeleteAllWithoutChildrenOptionStillDeletesNodetestDeleteChildrenOnlyCliRejectsRoottestDeleteAllRootWithoutChildrenOptionIsUnchangedThe 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 three independent reviewers (Claude Opus 5.5, GPT-5.5, GPT-6 Sol). Two reviews found issues (handling of
/as the target path, the root not being re-checked after-c, a docs example error, wrong minimum server version), all fixed in the second commit. A later PR review comment (deleteall / -ccould silently succeed on a large unchrooted root) was validated with a repro and by two further model reviews (Claude Opus 5.5, GPT-6.1 Sol), and fixed in the third commit.A final self-review pass (Claude Opus 5.5 and GPT-6.1 Sol, each asked to re-derive every claim from source) confirmed that nothing in the existing behavior changes, and produced the fourth and fifth commits: narrowing the new
listSubTreeBFSoverload to package-private so the new public API surface is unchanged, replacing the misleading-cfailure message, and correcting three documentation claims that were overstated (thejute.maxbufferguarantee, the justification for the root restriction, and the claim that a concurrent create could never be reported as success).These reviews ran outside the branch-review receipt flow, so the box is left unchecked.
Changes that Break Backward Compatibility (Optional)
None.
ZKUtil.deleteRecursive,ZKUtil.listSubTreeBFS(zk, path)anddeleteallwithout-cbehave exactly as before (samegetChildrencall, same delete order and batching). The new three-argumentlistSubTreeBFSoverload is package-private, so the public API surface is unchanged apart from the newdeleteChildrenRecursivemethod.-cwas previously rejected as an unrecognized option, and thedeleteallusage string now includes[-c].-crequires a server that supports paginatedgetChildren(fork3.6.3-1or later). Against an older server the command fails withUnimplementedbefore deleting anything.Documentation (Optional)
Updated
zookeeper-docs/src/main/resources/markdown/zookeeperCLI.md(deleteallusage, a-c/-bexample, and the/restriction).🤖 Generated with GitHub Copilot CLI