hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daryn Sharp (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-1788) FsShell ls: Show symlinks properties
Date Wed, 15 Jun 2011 21:56:47 GMT

    [ 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

        

Mime
View raw message