Henry Robinson commented on ZOOKEEPER-729:

Patch looks pretty good! A few nits:

1. Your tab level seems to be set to two spaces, but the standard is to use 
four tabs per space
2. I would change the LOG levels to DEBUG instead of INFO.
3. Consider calling listSubTree listSubTreeBFS? I agree about making the 
strategy explicit, I feel this is the best way to do it. 
4. You should be explicit in the listSubTree comment that this is *not* an 
atomic snapshot and does not necessarily represent the state of the sub-tree as 
it ever was - particularly given that this is a public method. 
5. Similarly, explicitly note in the comments that you are deleting all nodes 
regardless of their version. 
6. Did you decide against including a -r option for delete? I'm +1 on that idea 
rather than rmr, but don't feel that strongly. 
7. I would remove the TODO comments - I'm not a huge fan of them (anymore :) ). 
There is no separator constant, the BFS thing should be taken care of by 
renaming and we must make a decision about dealing with the async requests - my 
take is just to do what you are doing and just run through the entire tree. 

Finally - we need a test for this! I know there's not a lot of coverage of 
ZooKeeper, which is not your fault - but I'm intending to be fairly militant 
about tests :) Check out e.g. ACLRootTest for a structure to get a server up 
and running and a client to connect to it. A couple of tests which verify the 
behaviour of both variants would be great. Ideally, we'd be able to test the 
behaviour when a create interleaves with a recursive delete, but that might 
still be hard to do without mocking up some of the code.

I'll be happy to commit this once these are sorted out, thanks for your 

> Recursively delete a znode  - zkCli.sh rmr /node
> ------------------------------------------------
>                 Key: ZOOKEEPER-729
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-729
>             Project: Zookeeper
>          Issue Type: New Feature
>          Components: java client
>            Reporter: Kay Kay
>            Assignee: Kay Kay
>             Fix For: 3.4.0
>         Attachments: ZOOKEEPER-729.patch, ZOOKEEPER-729.patch
> Recursively delete a given znode in zookeeper, from the command-line. 
> New operation "rmr" added to zkclient. 
> $ ./zkCli.sh rmr /node 

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to