[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-1962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14876657#comment-14876657
 ] 

Edward Ribeiro commented on ZOOKEEPER-1962:
-------------------------------------------

This issue is really cool! A few code review comments:

Rename the variable below to {{recursive}}:

{code}
boolean recurse = cl.hasOption("r");
{code}

{code}
   if (children.size() > 0) {
      out.println(printChildren(children));
   }
{code}

can be rewritten as:

{code}
   if (!children.isEmpty()) {
      out.println(printChildren(children));
   }
{code}

Also, I would change the code below

{code}
   for (int i = 0; i < children.size(); i++) {
         File child = new File(path, children.get(i));
         allChildren.add(child.getPath());
         allChildren.addAll(getAllChildren(child.getPath(), watch, stat));
   }
{code}

as 

{code}
  for (String child : children) {
         String absolutePath = path + "/" + child;
         allChildren.add(absolutePath);
         allChildren.addAll(getAllChildren(absolutePath, watch, stat));
  }
{code}

IMO, the File usage is expensive when we only want to concatenate the parent 
path with its children. Plus, looking at the source code of {{java.io.File}} he 
normalizes the Strings   passed (parent and child), and does this according to 
calls to the underlying file system ({{java.io.Win32FileSystem}} and  
{{java.io.UnixFileSystem}}).

*Finally, I would like to alert to something: jute.maxBuffer default value is 
1MB. This means that if the sum of all znode strings size is > 1MB the call 
will fail* as explained in this thread: 
http://zookeeper-user.578899.n2.nabble.com/Best-Practice-while-creating-znodes-td7579262.html

A possible solution would be to have an accumulator that sum up the size of the 
Strings and if it happens to reach near {{jute.maxBuffer}} limit then return 
the partial result with and indication that is not all the list (maybe putting 
"..."). Or, even better, leverage the use of paginated getChildren() once it 
becomes available: https://issues.apache.org/jira/browse/ZOOKEEPER-2260

> Add a CLI command to recursively list a znode and children
> ----------------------------------------------------------
>
>                 Key: ZOOKEEPER-1962
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1962
>             Project: ZooKeeper
>          Issue Type: New Feature
>          Components: java client
>    Affects Versions: 3.4.6
>            Reporter: Gautam Gopalakrishnan
>            Assignee: Gautam Gopalakrishnan
>            Priority: Minor
>             Fix For: 3.5.2, 3.6.0
>
>         Attachments: ZOOKEEPER-1962.diff, ZOOKEEPER-1962_v2.patch, 
> ZOOKEEPER-1962_v3.patch
>
>   Original Estimate: 24h
>  Remaining Estimate: 24h
>
> When troubleshooting applications where znodes can be multiple levels deep  
> (eg. HBase replication), it is handy to see all child znodes recursively 
> rather than run an ls for each node manually.
> So I propose adding an option to the "ls" command (-r) which will list all 
> child nodes under a given znode. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to