[
https://issues.apache.org/jira/browse/HDFS-5120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13761615#comment-13761615
]
Andrew Wang commented on HDFS-5120:
-----------------------------------
DistributedFileSystem
* listCachePools javadoc says CachePool, I think you meant CachePoolInfo
CachePool
* Unnecessary StringUtils import
* We moved DEFAULT_MODE from CachePool into the FsPermission class in
HDFS-5163, looks like a rebase error
* Same comment for MODE_FORMAT_STRING, let's just use FsPermission's toString.
* validateName is only called in one place, dubious if we need this as a
separate function.
DFSAdmin
* Unnecessary Arrays and Iterator imports
StringUtils
* need javadoc on new methods
* What's the reason for terminating on "--"? It's usually used to separate sets
of variadic positional args, but I don't see that in any of the new methods.
DFSAdmin
* I prefer the style of some other commands in this file, e.g.
{{SetSpaceQuotaCommand}}, which extend DFSAdminCommand and use
{{CommandFormat}} from Commons CLI to do the argument parsing. This might let
us skip adding the StringUtils methods.
* Honestly, it seems like *everything* in this file should use CommandFormat
and extend DFSAdminCommand. If you agree, I'll file a newbie JIRA for trunk.
* Commons CLI might solve some of this, but if not, can we split the packing
and unpacking of CachePoolInfo in modifyCachePool into new methods? Function is
kind of long right now.
DFSAdmin#listCachePool and #listCachePools
* I think we should table-print like {{ls -l}} output instead of printing the
field name for each pool. Should look cleaner. This will require updating the
doc too.
* I also don't think we need the non-verbose mode, or it should be inverted as
{{-quiet}}, since we're fetching the extra data regardless (unlike {{ls -l}}).
* We can use a positional argument rather than a named one for the {{name}},
again like our friend {{ls}}
* Let's use a StringBuilder, build the entire format string, format once, print
once, rather than doing so much {{String.format}}.
* Should say "cache pools" rather than just "pools" in printlns.
DFSAdmin nits:
* extra space in {{<owner >}} in {{ADD_CACHE_POOL_USAGE}}
* Please capitalize output lines, following convention;
{code}
System.err.print("can't understand arguments: " +
System.err.println("usage is " + ADD_CACHE_POOL_USAGE);
System.err.print("can't understand arguments: " +
System.err.print("can't understand arguments: " +
System.out.println("no pool named " + name + " found.");
System.out.println("no pools found.");
{code}
* Please use one space after a period in the help text, again per convention.
Test:
* Need tests for verbose listing and modify pool.
* Should validate success on delete via a {{listCachePools}} as well.
> add command-line support for manipulating cache pools
> -----------------------------------------------------
>
> Key: HDFS-5120
> URL: https://issues.apache.org/jira/browse/HDFS-5120
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, namenode
> Affects Versions: HDFS-4949
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Attachments: HDFS-5120-caching.001.patch, HDFS-5120-caching.002.patch
>
>
> We should add command-line support for creating, removing, and listing cache
> directives and manipulating cache pools.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
