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

Colin Patrick McCabe commented on HADOOP-8040:
----------------------------------------------

{code}
  protected static void checkNotSchemeWithRelative(final Path path)
  protected static void checkNotRelative(final Path path)
  public static String getPathWithoutSchemeAndAuthority(Path path)
{code}

Why are these static methods rather than instance methods?  They seem to just 
operate on a single path, and don't do anything smart when you pass null, so I 
can't see why you'd want them to be static.  I also don't see why you would 
want {{protected}} here, rather than package-private (i.e. no qualifier.).

Also, a function whose name begins with getPath... should return a path.  Let 
the caller call toString if he wants to.

{code}
    System.out.println("RLFS wd: " + workingDir);
{code}

Debug printout left in.

{code}
  private FileStatus getFileLinkStatusInternal(final Path f) throws IOException 
{
    String target = readLink(f);

    try {
      FileStatus fs = getFileStatus(f);
      // If f refers to a regular file or directory
      if (target.isEmpty()) {
        return fs;
      }
      // Otherwise f refers to a symlink
      return new FileStatus(fs.getLen(),
          false,
          fs.getReplication(),
          fs.getBlockSize(),
          fs.getModificationTime(),
          fs.getAccessTime(),
          fs.getPermission(),
          fs.getOwner(),
          fs.getGroup(),
          new Path(target),
          f);
{code}

Symlinks do not necessarily have the same owner as the file they point to.  
Here's any example:

{code}
cmccabe@keter:~/hadoop2> touch a
cmccabe@keter:~/hadoop2> ln -s a b
cmccabe@keter:~/hadoop2> sudo chown bob b
root's password:
cmccabe@keter:~/hadoop2> ls -l a b
-rw-r--r-- 1 bob     users 0 Jun 18 12:14 a
lrwxrwxrwx 1 cmccabe users 1 Jun 18 12:14 b -> a
{code}

Symlinks have a file mode (aka permission bits) which are independent of the 
files they point to.  On certain UNIXes (but not Linux), symlinks can have 
permissions other than 0777.  I'm not sure if you can get access to lstat from 
JDK6, or if you'll need JNI for this.

{code}
  final public FileStatus makeQualified(URI defaultUri, Path path) {
{code}

This is basically a copy constructor that also modifies 
{{HdfsFileStatus#path}}.  Are you sure it wouldn't be easier to just have a 
method to change the path of the object?  As new fields are added to 
HdfsFileStatus, it's going to be very, very easy for people to forget to change 
this function to copy them.  In fact, you seem to have left a field out here 
yourself-- fileId.  This is especially an issue when subclasses start coming 
into play.  All the problems that plague clone() plague this method too.  I 
also feel like having a mutator like setParentPath(URI defaultURI, Path parent) 
would be more efficient, as well as more maintainable.

FSLinkResolver: it seems like you only need one of these per function.  Since 
they are stateless, there isn't any point in creating garbage that you'll have 
to collect later.  Just make a static FSLinkResolver<ContentSummary> and use 
that in getContentSummary.  etc. etc.  Also, should MAX_PATH_LINKS be 
configurable?
                
> Add symlink support to FileSystem
> ---------------------------------
>
>                 Key: HADOOP-8040
>                 URL: https://issues.apache.org/jira/browse/HADOOP-8040
>             Project: Hadoop Common
>          Issue Type: New Feature
>          Components: fs
>    Affects Versions: 0.23.0, 3.0.0, 2.0.3-alpha
>            Reporter: Eli Collins
>            Assignee: Andrew Wang
>         Attachments: hadoop-8040-1.patch, hadoop-8040-2.patch, 
> hadoop-8040-3.patch, hadoop-8040-4.patch, hadoop-8040-5.patch
>
>
> HADOOP-6421 added symbolic links to FileContext. Resolving symlinks is done 
> on the client-side, and therefore requires client support. An HDFS symlink 
> (created by FileContext) when accessed by FileSystem will result in an 
> unhandled UnresolvedLinkException. Because not all users will migrate from 
> FileSystem to FileContext in lock step, and we want users of FileSystem to be 
> able to access all paths created by FileContext, we need to support symlink 
> resolution in FileSystem as well, to facilitate migration to FileContext.

--
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

Reply via email to