[
https://issues.apache.org/jira/browse/HDFS-1788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13050065#comment-13050065
]
Daryn Sharp commented on HDFS-1788:
-----------------------------------
Very nice! First off, this isn't a HDFS change. Maybe someone with admin
powers can move the jira to HADOOP, or close this and repost on HADOOP-6424.
In general, I'd really like to see {{statReal}} revert to {{stat}}. That will
greatly reduce the size of the patch. Since I'm a unix stalwart, I'd "prefer"
{{statLink}} be called {{lstat}}. In either case, it should be a method to
avoid increasing the load on the NN (see more below).
+FsShell+
* Make sure that renaming {{getFS()}} to {{getFC()}} didn't break {{DFSAdmin}},
or {{TestMRCLI}}, etc.
* I think the addition of {{FileContext.processDeleteOnExit()}} to {{close()}}
may cause problems. Ex. What happens if I have temp files open before running
a FsShell command? Won't this change cause the files to unexpectedly go "poof"?
+Ls+
* The new {{\-L}} flag is really implementing part of {{\-l}}. {{\-L}} is
supposed to replace the link with the name & attrs of its target. It would be
a nice option, but probably not strictly needed unless you are feeling
ambitious. Now might be a good time to make {{\-l}} generate output the way ls
is REALLY supposed to look. Otherwise altering the {{\-l}} output in the
future will be deemed incompatible. You might consider another jira...
+PathData+
* I'd recommend undoing as much as possible since there reasons why it is the
way it is, plus it will cause a major merge conflict with my pending PathData
changes.
* The String ctors need to be restored since Path can mangle the string it is
given.
* The 3-arg ctor that takes FileStatus must be re-added. I took great effort
to reduce the RPC load on the NN, but this patch will undo some of that work
and generate *2X the stats to the NN*.
* Always doing the equivalent of stat/lstat on every object is causing *2X the
stats to the NN*. Combined with the previous point, this patch is causing *4X
the stats to the NN* unless there's magic going on deep in the client
* {{lstat}} is used so infrequently it should probably be an on-demand
{{item.lstat()}} method
* {{refreshStatus}} is expected to throw FNF, but this changes it to ignore it
* If {{FileSystem}} can't be completely eliminated, please remove
{{fs(Configuration)}} and leave the {{fs}} member intact. That will also
greatly reduce the size of the patch, and remove errors that may be caused by
providing a different config than the one used to originally create the object.
+Tail+
* Changing {{refreshStatus}} to not throw FNF will cause a NPE here. This is
one of the reasons {{refreshStatus}} needs to retain original behavior.
+CopyCommands+
* Are you sure {{copyCrcToLocal}} will work now? The raw fs was needed since
the actual fs obscures the crc file. Does {{FileContext}} change that behavior?
+Ln+
* {{Ln}} Shouldn't be in {{CopyCommands}}. Please move this class into it's
own file.
* Should require {{\-s}} to create symbolic links. If {{\-s}} isn't given, it
should throw that hardlinks aren't supported -- that way we leave the door open
to the possibility of hardlinks someday.
* It can't be a {{CommandWithDestination}} or it forces the target of the
symlink to exist at creation time. It's completely legit to create a symlink
that points to a non-existent path. Also, it should take 1 or 2 args like the
unix command. {{processArguments}} is probably the best place to implement it.
* Might consider splitting this off to accelerate the rest being integrated.
Up to you.
+LocalFileSystem+
* Why expose {{NAME}}? It doesn't appear to be used.
+FileContext+
* Why add {{fsUri}}? It doesn't appear to be used.
Overall, great work! Don't let the length of the review be discouraging. It's
a great improvement to the shell!
> FsShell ls: Show symlinks properties
> ------------------------------------
>
> Key: HDFS-1788
> URL: https://issues.apache.org/jira/browse/HDFS-1788
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: tools
> Reporter: Jonathan Eagles
> Assignee: John George
> Priority: Minor
> Attachments: HDFS-1788.patch
>
>
> ls FsShell command implementation has been consistent with the linux
> implementations of ls \-l. With the addition of symlinks, I would expect the
> ability to show file type 'd' for directory, '\-' for file, and 'l' for
> symlink. In addition, following the linkname entry for symlinks, I would
> expect the ability to show "\-> <link target>". In linux, the default is to
> the the properties of the link and not of the link target. In linux, '-L'
> option allows for the dereferencing of symlinks to show link target
> properties, but it is not the default.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
