Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
       Adds Symbolic links to FileContext, AbstractFileSystem.
       It also adds a limited implementation for the local file system (RawLocalFs) that allows local symlinks.
      Breaks compatibility: FileStatus#isDir() if used to determine if the object is a file will be incorrect if the object happens to be a symlink.
      Show
       Adds Symbolic links to FileContext, AbstractFileSystem.  It also adds a limited implementation for the local file system (RawLocalFs) that allows local symlinks. Breaks compatibility: FileStatus#isDir() if used to determine if the object is a file will be incorrect if the object happens to be a symlink.

      Description

      Here's a jira for the common parts of HDFS-245, mostly changes to FileContext and AbstractFileSystem.

      1. symlink24-common.patch
        34 kB
        Eli Collins
      2. symlink-25-common.patch
        48 kB
        Eli Collins
      3. symlink-26-common.patch
        48 kB
        Eli Collins
      4. symlink-26-common.patch
        49 kB
        Eli Collins
      5. symlink27-common.patch
        59 kB
        Eli Collins
      6. symlink28-common.patch
        48 kB
        Eli Collins
      7. symlink29-common.patch
        50 kB
        Eli Collins
      8. symlink29-common.patch
        50 kB
        Eli Collins
      9. symlink29-common.patch
        50 kB
        Eli Collins
      10. symlink30-common.patch
        51 kB
        Eli Collins
      11. symlink31-common.patch
        74 kB
        Eli Collins
      12. symlink32-common.patch
        74 kB
        Eli Collins
      13. symlink33-common.patch
        75 kB
        Eli Collins
      14. symlink34-common.patch
        79 kB
        Eli Collins
      15. symlink35-common.patch
        95 kB
        Eli Collins
      16. symlink36-common.patch
        92 kB
        Eli Collins
      17. symlink37-common.patch
        92 kB
        Eli Collins
      18. symlink38-common.patch
        97 kB
        Eli Collins
      19. symlink39-common.patch
        97 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          Latest patch attached.

          Show
          Eli Collins added a comment - Latest patch attached.
          Hide
          Doug Cutting added a comment -

          Should we add an implementation to LocalFileSystem here? That would enable us to add some tests. It could use shell commands to implement createSymlink() and getLinkTarget(). The hardest bit might be implementing listStatus() and getStatus() to correctly support isLink().

          Testing could instead be accomplished with a FilterFileSystem implementation used only for testing. It would be nice to have a usable implemenation for the local fs, but that could be left to a separate issue.

          Show
          Doug Cutting added a comment - Should we add an implementation to LocalFileSystem here? That would enable us to add some tests. It could use shell commands to implement createSymlink() and getLinkTarget(). The hardest bit might be implementing listStatus() and getStatus() to correctly support isLink(). Testing could instead be accomplished with a FilterFileSystem implementation used only for testing. It would be nice to have a usable implemenation for the local fs, but that could be left to a separate issue.
          Hide
          Eli Collins added a comment -

          I'm actually working on adding them to LocalFs today. Do you think we need LocalFileSystem as well? I uploaded a test plan to HDFS-254 yesterday, the things marked xxx are what I'm currently working on that block further testing (like LocalFs).

          Show
          Eli Collins added a comment - I'm actually working on adding them to LocalFs today. Do you think we need LocalFileSystem as well? I uploaded a test plan to HDFS-254 yesterday, the things marked xxx are what I'm currently working on that block further testing (like LocalFs).
          Hide
          Doug Cutting added a comment -

          No, I meant LocalFs. Sounds good.

          Show
          Doug Cutting added a comment - No, I meant LocalFs. Sounds good.
          Hide
          Eli Collins added a comment -

          Broke out the path part of this change to HADOOP-6427.

          Show
          Eli Collins added a comment - Broke out the path part of this change to HADOOP-6427 .
          Hide
          Eli Collins added a comment -

          Attached the latest patch, includes a symlink implementation for LocalFs and more tests.

          Show
          Eli Collins added a comment - Attached the latest patch, includes a symlink implementation for LocalFs and more tests.
          Hide
          Eli Collins added a comment -

          By the way, I ended up shelling out to implement createSymlink, getLinkTarget and getFileLinkStatus. I also wrote an implementation that avoids this by making a NativeCall class that makes jni calls to link, readlink etc but integrating it into the native build was a pain. Does anyone know why all the auto-generated build files are checked in to src/native?

          Show
          Eli Collins added a comment - By the way, I ended up shelling out to implement createSymlink, getLinkTarget and getFileLinkStatus. I also wrote an implementation that avoids this by making a NativeCall class that makes jni calls to link , readlink etc but integrating it into the native build was a pain. Does anyone know why all the auto-generated build files are checked in to src/native?
          Hide
          Doug Cutting added a comment -

          > Does anyone know why all the auto-generated build files are checked in to src/native?

          I think it's so that developers don't have to have automake etc. installed. Seems like a strange optimization though. Owen?

          Show
          Doug Cutting added a comment - > Does anyone know why all the auto-generated build files are checked in to src/native? I think it's so that developers don't have to have automake etc. installed. Seems like a strange optimization though. Owen?
          Hide
          Owen O'Malley added a comment -

          It isn't an optimization as much as making sure more people can compile the system.

          At this point, most platforms that we care about, it is easy to install autoconf and automake. If someone wants to open a jira that deletes the files (and moves them into build!) that would be great. It would make the auto line counting tools stop thinking that Hadoop is a project dominated by shell scripts. smile

          Unlike the days of old, I would not even include the autoconf/automake generated stuff in the source tarball.

          Show
          Owen O'Malley added a comment - It isn't an optimization as much as making sure more people can compile the system. At this point, most platforms that we care about, it is easy to install autoconf and automake. If someone wants to open a jira that deletes the files (and moves them into build!) that would be great. It would make the auto line counting tools stop thinking that Hadoop is a project dominated by shell scripts. smile Unlike the days of old, I would not even include the autoconf/automake generated stuff in the source tarball.
          Hide
          Eli Collins added a comment -

          Excellent. I filed HADOOP-6436. Will take a stab at it while waiting for reviews on symlink patches =)

          Show
          Eli Collins added a comment - Excellent. I filed HADOOP-6436 . Will take a stab at it while waiting for reviews on symlink patches =)
          Hide
          Eli Collins added a comment -

          Latest patch. Addresses a test failure from test-core and adds some tests.

          Show
          Eli Collins added a comment - Latest patch. Addresses a test failure from test-core and adds some tests.
          Hide
          Doug Cutting added a comment -

          This includes HADOOP-6427 code. Is that intentional? Should this depend on that issue?

          Show
          Doug Cutting added a comment - This includes HADOOP-6427 code. Is that intentional? Should this depend on that issue?
          Hide
          Eli Collins added a comment -

          Yup, that's intentional. I've linked the issues.

          Show
          Eli Collins added a comment - Yup, that's intentional. I've linked the issues.
          Hide
          Eli Collins added a comment -

          The isQualified test is an error check. Ideally the namenode resolves all links within the file system so all the UnresolvedLinkExceptions that FileContext sees have fully qualified targets. In the current patch the namenode throws an exception to the client even if the link is within the same file system, but even then we want to check that we got back a fully qualified path be clear that the interpretation of the link target is done by the AbstractFileSystem and not FileContext. So I think we need a function to determine if a path is fully qualified in any case right? It also serves to define what we mean by "fully qualified" in Path (this is currently defined implicitly in the makeQualfied method).

          I took your (excellent) suggestion of storing the link target verbatim but have the client return a fully qualified path. That's definitely the way to go. The current semantics follow, will update the design doc. All of these cases are now covered in individual tests in TestLink and TestLocalFsLink.

          • Relative link targets

          fc.createSymlink("file", "/dir/link") creates a link named "link" in /dir (assuming the current working directory is "/dir") that points to "file", eg resolves to "/dir/file", and if "dir" is renamed "dir2" the link resolves to "/dir2/link" because the path is not stored relative. The file context is determined by the parent of the link, eg the fully qualified path of the link target of "/dir/link" is determined by "/dir" not the file context that is used to access the link.

          • Absolute links targets

          fc.createSymlink("/dir/file", "/dir/link") creates a link named "link" in /dir that points to "/dir/file", if "dir" is renamed "dir2" the link becomes dangling because the link target is stored absolute. The file system is determined by the source not the client, eg fc.open("hdfs://host1/dir/link") opens "/dir/file" on host1 even if accessed using a local file context.

          • Fully qualified link targets

          fc.createSymlink("hdfs://host/dir/file", "/dir/link") creates a link named "link" in /dir that always points to the fully qualified path specified, regardless of the file context or path used to access the link.

          Show
          Eli Collins added a comment - The isQualified test is an error check. Ideally the namenode resolves all links within the file system so all the UnresolvedLinkExceptions that FileContext sees have fully qualified targets. In the current patch the namenode throws an exception to the client even if the link is within the same file system, but even then we want to check that we got back a fully qualified path be clear that the interpretation of the link target is done by the AbstractFileSystem and not FileContext. So I think we need a function to determine if a path is fully qualified in any case right? It also serves to define what we mean by "fully qualified" in Path (this is currently defined implicitly in the makeQualfied method). I took your (excellent) suggestion of storing the link target verbatim but have the client return a fully qualified path. That's definitely the way to go. The current semantics follow, will update the design doc. All of these cases are now covered in individual tests in TestLink and TestLocalFsLink. Relative link targets fc.createSymlink("file", "/dir/link") creates a link named "link" in /dir (assuming the current working directory is "/dir") that points to "file", eg resolves to "/dir/file", and if "dir" is renamed "dir2" the link resolves to "/dir2/link" because the path is not stored relative. The file context is determined by the parent of the link, eg the fully qualified path of the link target of "/dir/link" is determined by "/dir" not the file context that is used to access the link. Absolute links targets fc.createSymlink("/dir/file", "/dir/link") creates a link named "link" in /dir that points to "/dir/file", if "dir" is renamed "dir2" the link becomes dangling because the link target is stored absolute. The file system is determined by the source not the client, eg fc.open("hdfs://host1/dir/link") opens "/dir/file" on host1 even if accessed using a local file context. Fully qualified link targets fc.createSymlink("hdfs://host/dir/file", "/dir/link") creates a link named "link" in /dir that always points to the fully qualified path specified, regardless of the file context or path used to access the link.
          Hide
          Eli Collins added a comment -

          Latest patch implements the semantics and adds tests according to the previous update.

          Show
          Eli Collins added a comment - Latest patch implements the semantics and adds tests according to the previous update.
          Hide
          dhruba borthakur added a comment -

          Your latest desciption of relative/absolute/fullyqualified link targets make sense. +1.

          Show
          dhruba borthakur added a comment - Your latest desciption of relative/absolute/fullyqualified link targets make sense. +1.
          Hide
          Eli Collins added a comment -

          Hey Dhruba,

          Just to confirm, you mean the description posted in HDFS-245 right?
          Which is different from the one in the comment in HADOOP-6421.

          Thanks,
          Eli

          On Fri, Dec 18, 2009 at 6:23 AM, dhruba borthakur (JIRA)

          Show
          Eli Collins added a comment - Hey Dhruba, Just to confirm, you mean the description posted in HDFS-245 right? Which is different from the one in the comment in HADOOP-6421 . Thanks, Eli On Fri, Dec 18, 2009 at 6:23 AM, dhruba borthakur (JIRA)
          Hide
          Eli Collins added a comment -

          Attached latest patch.

          • Implements path resolution as discussed in HDFS-245 and HADOOP-6427
          • Added an interface to AbstractFileSystem to indicate if the file system supports symlinks, and a test in FileContextMainOperationsBaseTest to test the behavior of createSymlink when the file system does not support links.
          • Additional tests in TestLocalFsLink
          • Resolved against trunk, passes test-core
          • Removed the changes to HarFileSystem and FsShell so the APIs are only accessible via the tests.
          Show
          Eli Collins added a comment - Attached latest patch. Implements path resolution as discussed in HDFS-245 and HADOOP-6427 Added an interface to AbstractFileSystem to indicate if the file system supports symlinks, and a test in FileContextMainOperationsBaseTest to test the behavior of createSymlink when the file system does not support links. Additional tests in TestLocalFsLink Resolved against trunk, passes test-core Removed the changes to HarFileSystem and FsShell so the APIs are only accessible via the tests.
          Hide
          Eli Collins added a comment -

          Here's the latest patch that implements the resolution behavior posted to HDFS-245 that Doug, Sanjay and I discussed. Rebased against trunk and passes test-core.

          Show
          Eli Collins added a comment - Here's the latest patch that implements the resolution behavior posted to HDFS-245 that Doug, Sanjay and I discussed. Rebased against trunk and passes test-core.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429579/symlink29-common.patch
          against trunk revision 896641.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 50 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 3 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429579/symlink29-common.patch against trunk revision 896641. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 50 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 3 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Update the last patch to address the find bugs warnings, replaced instantiating a Boolean with Boolean.valueOf in three places in FileContext.

          Show
          Eli Collins added a comment - Update the last patch to address the find bugs warnings, replaced instantiating a Boolean with Boolean.valueOf in three places in FileContext.
          Hide
          Eli Collins added a comment -

          Minor update to address javadoc warnings and checkstyle suggestions.

          Show
          Eli Collins added a comment - Minor update to address javadoc warnings and checkstyle suggestions.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429601/symlink29-common.patch
          against trunk revision 896691.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 50 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429601/symlink29-common.patch against trunk revision 896691. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 50 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429611/symlink29-common.patch
          against trunk revision 896691.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 50 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429611/symlink29-common.patch against trunk revision 896691. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 50 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Not sure what the javadoc warning is about, ant javadoc and ant javadoc-dev are both clean on my host and I've pulled the latest from trunk.

          Show
          Eli Collins added a comment - Not sure what the javadoc warning is about, ant javadoc and ant javadoc-dev are both clean on my host and I've pulled the latest from trunk.
          Hide
          Eli Collins added a comment -

          Same patch different name. Hudson used the old copy of the previous patch.

          Show
          Eli Collins added a comment - Same patch different name. Hudson used the old copy of the previous patch.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429620/symlink30-common.patch
          against trunk revision 896691.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 50 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429620/symlink30-common.patch against trunk revision 896691. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 50 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/console This message is automatically generated.
          Hide
          Sanjay Radia added a comment -
          • DeletegateToFileSystem
            shouldn't createSymlink and getLinkTarget throw an exception?
          • FilterFs - typo
            setTimes() added a blank line.
          • TestLocalFsLink
            The following two should be in setUp and tearDown respectively; will make writing new tests easier and will also be less error prone (e.g. so that delete is not forgotten).
            • Path dir = new Path("file:///tmp/test"); // should be in setUp
            • fc.delete(dir, true); // should be in tearDown
          • TestLocalFsLink
            Suggestion: follow the other tests fby setting up a base test and have subclasses that implement the tests for specific file systems.
            For examples see: FileContextCreateMkdirBaseTest and TestLocalFSFileContextCreateMkdir).
            I noticed that the symlink tests for HDFS are significantly more comprehensive. Would it have been possible to share the tests for both file systems
            using the base and subclasses as in other tests?

          +1 Looks good except for the above. Will get to your other symlink related jiras in next 2-3 days.

          Show
          Sanjay Radia added a comment - DeletegateToFileSystem shouldn't createSymlink and getLinkTarget throw an exception? FilterFs - typo setTimes() added a blank line. TestLocalFsLink The following two should be in setUp and tearDown respectively; will make writing new tests easier and will also be less error prone (e.g. so that delete is not forgotten). Path dir = new Path("file:///tmp/test"); // should be in setUp fc.delete(dir, true); // should be in tearDown TestLocalFsLink Suggestion: follow the other tests fby setting up a base test and have subclasses that implement the tests for specific file systems. For examples see: FileContextCreateMkdirBaseTest and TestLocalFSFileContextCreateMkdir). I noticed that the symlink tests for HDFS are significantly more comprehensive. Would it have been possible to share the tests for both file systems using the base and subclasses as in other tests? +1 Looks good except for the above. Will get to your other symlink related jiras in next 2-3 days.
          Hide
          Eli Collins added a comment -

          DeletegateToFileSystem. shouldn't createSymlink and getLinkTarget throw an exception?

          Good catch, they now both throw an IOException indicating symlinks are not supported. For uniformity, I made getFileLinkStatus do the same, and modified createSymlink to pass the IOException up rather than return a boolean. This way if the user tries to create or access a symlink on a file system that does not support them an IOException is thrown in all cases. I also added tests for getLinkTarget and getFileLinkStatus to testUnsupportedSymlink to verify the behavior for each method when links are unsupported. All relevant javadocs updated.

          FilterFs - typo setTimes() added a blank line.

          The blank line was already there, the patch removes it.

          TestLocalFsLink. The following two should be in setUp and tearDown respectively;

          Fixed. Removed empty method clusterTeardown (was @AfterClass) and added cleanupDirs (@After).

          TestLocalFsLink. Suggestion: follow the other tests fby setting up a base test and have subclasses that implement the tests for specific file systems.

          Done. I created FileContextSymlinkBaseTest and made the test classes for the local file system and HDFS extend it.

          I noticed that the symlink tests for HDFS are significantly more comprehensive. Would it have been possible to share the tests for both file systems using the base and subclasses as in other tests?

          Yup. With some refactoring I was able to move the bulk of the tests to the common class. It has 33 tests, the local file system class has 4, and the HDFS class now has 13. This uncovered an additional change I needed to make in AbstractFileSystem#rename to avoid throwing a ParentNotDirectoryException if the parent is a symlink.

          Thanks for the review! I updated the diffs over on HDFS-245 as well.

          Show
          Eli Collins added a comment - DeletegateToFileSystem. shouldn't createSymlink and getLinkTarget throw an exception? Good catch, they now both throw an IOException indicating symlinks are not supported. For uniformity, I made getFileLinkStatus do the same, and modified createSymlink to pass the IOException up rather than return a boolean. This way if the user tries to create or access a symlink on a file system that does not support them an IOException is thrown in all cases. I also added tests for getLinkTarget and getFileLinkStatus to testUnsupportedSymlink to verify the behavior for each method when links are unsupported. All relevant javadocs updated. FilterFs - typo setTimes() added a blank line. The blank line was already there, the patch removes it. TestLocalFsLink. The following two should be in setUp and tearDown respectively; Fixed. Removed empty method clusterTeardown (was @AfterClass) and added cleanupDirs (@After). TestLocalFsLink. Suggestion: follow the other tests fby setting up a base test and have subclasses that implement the tests for specific file systems. Done. I created FileContextSymlinkBaseTest and made the test classes for the local file system and HDFS extend it. I noticed that the symlink tests for HDFS are significantly more comprehensive. Would it have been possible to share the tests for both file systems using the base and subclasses as in other tests? Yup. With some refactoring I was able to move the bulk of the tests to the common class. It has 33 tests, the local file system class has 4, and the HDFS class now has 13. This uncovered an additional change I needed to make in AbstractFileSystem#rename to avoid throwing a ParentNotDirectoryException if the parent is a symlink. Thanks for the review! I updated the diffs over on HDFS-245 as well.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429987/symlink31-common.patch
          against trunk revision 897023.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 12 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429987/symlink31-common.patch against trunk revision 897023. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          This may refer to an already-committed JIRA, but it seems odd to me that getFileLinkStatus throws when called on a non-link-supporting system. Isn't the idea to replicate lstat semantics? lstat on a non-link is identical to stat, and I'd expect the same of getFileLinkStatus.

          Show
          Todd Lipcon added a comment - This may refer to an already-committed JIRA, but it seems odd to me that getFileLinkStatus throws when called on a non-link-supporting system. Isn't the idea to replicate lstat semantics? lstat on a non-link is identical to stat, and I'd expect the same of getFileLinkStatus.
          Hide
          Eli Collins added a comment -

          Nice catch Todd, was over zealous in my uniformity. Here's the behavior I proposed in HDFS-245:

          getLinkFileStatus - like lstat, if the given path refers to a symlink then the FileStatus of the symlink is returned, otherwise the results as if getFileStatus was called. If symlink support is not enabled or the underlying filesystem does not support symlinks then the results are the same as if getFileStatus was called.

          The attached patch reverts to the old behavior that getFileLinkStatus is equivalent to getFileStatus if the underlying file system does not support symlinks.

          Show
          Eli Collins added a comment - Nice catch Todd, was over zealous in my uniformity. Here's the behavior I proposed in HDFS-245 : getLinkFileStatus - like lstat, if the given path refers to a symlink then the FileStatus of the symlink is returned, otherwise the results as if getFileStatus was called. If symlink support is not enabled or the underlying filesystem does not support symlinks then the results are the same as if getFileStatus was called. The attached patch reverts to the old behavior that getFileLinkStatus is equivalent to getFileStatus if the underlying file system does not support symlinks.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429997/symlink32-common.patch
          against trunk revision 897023.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 15 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429997/symlink32-common.patch against trunk revision 897023. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          While updating the design doc I noticed a missing test. This patch adds testLinkStatusAndTargetWithNonLink to show the behavior of getLinkTarget
          when called on a normal file (throws an IOException ala how
          readlink returns EINVAL in this case). I also made getLinkTarget in
          DelegateToFileSystem throw an AssertionError since that method should
          never be called--any file system implementation that supports
          symlinks and throws an UnresolvedLinkException needs to override
          getLinkTarget.

          Show
          Eli Collins added a comment - While updating the design doc I noticed a missing test. This patch adds testLinkStatusAndTargetWithNonLink to show the behavior of getLinkTarget when called on a normal file (throws an IOException ala how readlink returns EINVAL in this case). I also made getLinkTarget in DelegateToFileSystem throw an AssertionError since that method should never be called--any file system implementation that supports symlinks and throws an UnresolvedLinkException needs to override getLinkTarget.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430209/symlink33-common.patch
          against trunk revision 898740.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 15 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12430209/symlink33-common.patch against trunk revision 898740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/console This message is automatically generated.
          Hide
          Sanjay Radia added a comment -

          Oops. I fogot to copy one of my feedback items from my notes.

          • createSymlink
            • should follow the create() and mkdir() methods of providing a parameter to indicate if missing parent dirs should be created.
            • return parameter - somewhat religious - my preference is for the return to be void. Matches mkdir.
          Show
          Sanjay Radia added a comment - Oops. I fogot to copy one of my feedback items from my notes. createSymlink should follow the create() and mkdir() methods of providing a parameter to indicate if missing parent dirs should be created. return parameter - somewhat religious - my preference is for the return to be void. Matches mkdir.
          Hide
          Sanjay Radia added a comment -

          Javadoc for createSymlink.
          I realize that you are writing a document for the spec and usage model for sym links, however, I think the javadoc for createSymlink should be reasonably complete. I suggest that you include the following:

          • kinds of symlinks (to another FS, dot relative, volume-root relative etc.)
          • [dangling links are allowed.]
          • permissions
            • the permissions for the link itself is really that of the directory in which it sits. Hence you can change it if you have permission on the directory
            • what permissions apply when you follow links.
          • A statement on how the methods deal with a symlink. You have an nice writeup in HDFS-245 (comment dated 15/oct/09). My personal preference is to describe what the other methods do when crossing a symlink in the createSymlink javadoc rather than the the javadoc of the methods themselves.
          Show
          Sanjay Radia added a comment - Javadoc for createSymlink. I realize that you are writing a document for the spec and usage model for sym links, however, I think the javadoc for createSymlink should be reasonably complete. I suggest that you include the following: kinds of symlinks (to another FS, dot relative, volume-root relative etc.) [dangling links are allowed.] permissions the permissions for the link itself is really that of the directory in which it sits. Hence you can change it if you have permission on the directory what permissions apply when you follow links. A statement on how the methods deal with a symlink. You have an nice writeup in HDFS-245 (comment dated 15/oct/09). My personal preference is to describe what the other methods do when crossing a symlink in the createSymlink javadoc rather than the the javadoc of the methods themselves.
          Hide
          Allen Wittenauer added a comment -

          the permissions for the link itself is really that of the directory in which it sits. Hence you can change it if you have permission on the directory

          FWIW, I (still) think not having real ownership/permissions on symlinks is going to be a bad idea in the long run.
          [My standard example still holds: Take for example a full HDFS. As an admin, I want to point one or two files/dirs in that dir to somewhere else but leave the rest of the dir writable. This means that any user can change the link and break something that they shouldn't be able to break.]

          Show
          Allen Wittenauer added a comment - the permissions for the link itself is really that of the directory in which it sits. Hence you can change it if you have permission on the directory FWIW, I (still) think not having real ownership/permissions on symlinks is going to be a bad idea in the long run. [My standard example still holds: Take for example a full HDFS. As an admin, I want to point one or two files/dirs in that dir to somewhere else but leave the rest of the dir writable. This means that any user can change the link and break something that they shouldn't be able to break.]
          Hide
          Eli Collins added a comment -

          I think that's reasonable and I'd be willing to implement lchown in a follow on jira. Though I think we should leave the deafault perm behavior as is.

          Show
          Eli Collins added a comment - I think that's reasonable and I'd be willing to implement lchown in a follow on jira. Though I think we should leave the deafault perm behavior as is.
          Hide
          Eli Collins added a comment -

          Hey Sanjay,

          createSymlink
          should follow the create() and mkdir() methods of providing a parameter to indicate if missing parent dirs should be created.

          Agreed, will do. I'm not going to add a convenience method that doesn't take the createParent argument.

          return parameter - somewhat religious - my preference is for the return to be void. Matches mkdir.

          Changed that in the previous patch, it returns void.

          Will update the java docs per the above. The latest description of the new APIs follows, will update the design doc.

          Thanks,
          Eli

          FileContext

          • FileContext#getLinkFileStatus – If the final path component refers to a symlink then the FileStatus of the symlink is returned. If the the final path component does not refer to a symlink, or the underlying filesystem does not support symlinks, then the result is the same as if getFileStatus was called (like lstat).
          • FileContext#getLinkTarget – If the final path component refers to a symlink then the target of the symlink is returned. Links within the path leading up to the final path component are resolved transparently. The target returned is exactly as it was specified in createSymlink. If the the final path component does not refer to a symlink then an IOException is thrown (like readlink which returns EINVAL if the named file is not a symbolic link).
          • FileContext#createSymlink – Creates a symbolic link to an existing file. An exception is thrown if the link exits, the user does not have permission to create link, or the underlying file system does not support symlinks.

          FileStatus

          • FileStatus#isSymlink – Returns true if the FileStatus represents a symbolic link.
          • FileStatus#getSymlink – If the FileStatus represents a sylink then the target of the symlink is returned. The symlink target returned is exactly as it was specified in createSymlink. Otherwise an IOException is thrown indicating the FileStatus does not refer to a symlink.

          AbstractFileSystem

          • AbstractFileSystem#supportsSymlinks – Returns true if the file system supports symlinks, false otherwise.
          • AbstractFileSystem#createSymlink – Like FileContext#createSymlink. An IOException is thrown if the file system does not support symlinks.
          • AbstractFileSystem#getLinkTarget – Like FileContext#createSymlink. This method is only called by FileContext in response to an UnresolvedLinkException.
          • AbstractFileSystem#getFileLinkStatus – Like FileContext#getFileLinkStatus. Except that an UnresolvedLinkException may be thrown if a symlink is encountered in the path leading up to the final path component.
          Show
          Eli Collins added a comment - Hey Sanjay, createSymlink should follow the create() and mkdir() methods of providing a parameter to indicate if missing parent dirs should be created. Agreed, will do. I'm not going to add a convenience method that doesn't take the createParent argument. return parameter - somewhat religious - my preference is for the return to be void. Matches mkdir. Changed that in the previous patch, it returns void. Will update the java docs per the above. The latest description of the new APIs follows, will update the design doc. Thanks, Eli FileContext FileContext#getLinkFileStatus – If the final path component refers to a symlink then the FileStatus of the symlink is returned. If the the final path component does not refer to a symlink, or the underlying filesystem does not support symlinks, then the result is the same as if getFileStatus was called (like lstat ). FileContext#getLinkTarget – If the final path component refers to a symlink then the target of the symlink is returned. Links within the path leading up to the final path component are resolved transparently. The target returned is exactly as it was specified in createSymlink. If the the final path component does not refer to a symlink then an IOException is thrown (like readlink which returns EINVAL if the named file is not a symbolic link). FileContext#createSymlink – Creates a symbolic link to an existing file. An exception is thrown if the link exits, the user does not have permission to create link, or the underlying file system does not support symlinks. FileStatus FileStatus#isSymlink – Returns true if the FileStatus represents a symbolic link. FileStatus#getSymlink – If the FileStatus represents a sylink then the target of the symlink is returned. The symlink target returned is exactly as it was specified in createSymlink. Otherwise an IOException is thrown indicating the FileStatus does not refer to a symlink. AbstractFileSystem AbstractFileSystem#supportsSymlinks – Returns true if the file system supports symlinks, false otherwise. AbstractFileSystem#createSymlink – Like FileContext#createSymlink. An IOException is thrown if the file system does not support symlinks. AbstractFileSystem#getLinkTarget – Like FileContext#createSymlink. This method is only called by FileContext in response to an UnresolvedLinkException. AbstractFileSystem#getFileLinkStatus – Like FileContext#getFileLinkStatus. Except that an UnresolvedLinkException may be thrown if a symlink is encountered in the path leading up to the final path component.
          Hide
          dhruba borthakur added a comment -

          > FWIW, I (still) think not having real ownership/permissions on symlinks is going to be a bad idea in the long run.

          I think we can defer this to a follow on JIRA.

          Show
          dhruba borthakur added a comment - > FWIW, I (still) think not having real ownership/permissions on symlinks is going to be a bad idea in the long run. I think we can defer this to a follow on JIRA.
          Hide
          Eli Collins added a comment -

          Patch attached. FileContext#createSymlink now has a createParent parameter. I left the directory (and link) permission out of the FileContext interface, don't think it's complex enough to justify CreateOpts. I did plumb the link permissions up to ClientProtocol so if we wanted to have FileContext#createSymlink specify link permissions that would be a client side only change. I also added a common symlink test to verify that the parent is created when createParent is true and not when createParent is false. Updated the javadocs per your comment and to reflect the API change; ant javadoc and javadoc-dev are clean. Rebased against the latest bits from trunk.

          Show
          Eli Collins added a comment - Patch attached. FileContext#createSymlink now has a createParent parameter. I left the directory (and link) permission out of the FileContext interface, don't think it's complex enough to justify CreateOpts. I did plumb the link permissions up to ClientProtocol so if we wanted to have FileContext#createSymlink specify link permissions that would be a client side only change. I also added a common symlink test to verify that the parent is created when createParent is true and not when createParent is false. Updated the javadocs per your comment and to reflect the API change; ant javadoc and javadoc-dev are clean. Rebased against the latest bits from trunk.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430484/symlink34-common.patch
          against trunk revision 899866.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 15 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12430484/symlink34-common.patch against trunk revision 899866. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/console This message is automatically generated.
          Hide
          Jeff Hammerbacher added a comment -

          Is this patch ready to commit?

          Show
          Jeff Hammerbacher added a comment - Is this patch ready to commit?
          Hide
          Eli Collins added a comment -

          Yes, but it needs to be checked in simultaneously with the patch for HDFS-245 (otherwise the build will fail). Sanjay, have you gotten a chance to look at the HDFS-245 patch yet?

          Show
          Eli Collins added a comment - Yes, but it needs to be checked in simultaneously with the patch for HDFS-245 (otherwise the build will fail). Sanjay, have you gotten a chance to look at the HDFS-245 patch yet?
          Hide
          Eli Collins added a comment -

          Patch with latest common changes to match HDFS-245.

          Show
          Eli Collins added a comment - Patch with latest common changes to match HDFS-245 .
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431923/symlink35-common.patch
          against trunk revision 904975.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 15 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12431923/symlink35-common.patch against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Patch with latest common changes to match HDFS-245.

          Show
          Eli Collins added a comment - Patch with latest common changes to match HDFS-245 .
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434650/symlink36-common.patch
          against trunk revision 905860.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 14 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12434650/symlink36-common.patch against trunk revision 905860. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 14 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/console This message is automatically generated.
          Hide
          Sanjay Radia added a comment -

          Feedback on patch patch 33, FileContextSymlinkBaseTest

          • setup should create the test dir.
          • testCreateFileViaSymlink()
            it does not create the file via the symlink - file is created directly; it is only read via symlink.
          • testCreateFileExistingLink
            rename to testCreateFileDirExitingLink
            add a mkdir that fails.
          • testCreateLinkUsingRelPath
            rename testCreateLinkToFileUsingRelPath to match with testCreateLinkToFileUsingXXX
          • testCreateLinkToFileUsingXXX
            in testCreateLinkToFileUsingAbsPath you read the file "using rel, abs, and qualified paths"
            It would make sense to do that for the other two tests.
          • testSetWDResolvesLink
            Uses a link with an abs path.
            For this test it would worth adding test for all 3 symlinks (rel, abs, qual) since the
            code-path here is different than testCreateLinkUsingXXX and will not be covered by those tests.
          • Structure and coverage:
            Noticed that testCreateLinkXX has tests for abs, relative and FQ links.
            Most of the other tests use an abs link.
            You also have separate tests that test relative, abs, FQ links but they only test specific methods but not all.
            So I am somewhat unclear about the structure of the tests in how they cover all 3 kinds of links.
          • Use the test library - FileContextTestHelper.java - it will make your tests simpler.
          Show
          Sanjay Radia added a comment - Feedback on patch patch 33, FileContextSymlinkBaseTest setup should create the test dir. testCreateFileViaSymlink() it does not create the file via the symlink - file is created directly; it is only read via symlink. testCreateFileExistingLink rename to testCreateFileDirExitingLink add a mkdir that fails. testCreateLinkUsingRelPath rename testCreateLinkToFileUsingRelPath to match with testCreateLinkToFileUsingXXX testCreateLinkToFileUsingXXX in testCreateLinkToFileUsingAbsPath you read the file "using rel, abs, and qualified paths" It would make sense to do that for the other two tests. testSetWDResolvesLink Uses a link with an abs path. For this test it would worth adding test for all 3 symlinks (rel, abs, qual) since the code-path here is different than testCreateLinkUsingXXX and will not be covered by those tests. Structure and coverage: Noticed that testCreateLinkXX has tests for abs, relative and FQ links. Most of the other tests use an abs link. You also have separate tests that test relative, abs, FQ links but they only test specific methods but not all. So I am somewhat unclear about the structure of the tests in how they cover all 3 kinds of links. Use the test library - FileContextTestHelper.java - it will make your tests simpler.
          Hide
          Eli Collins added a comment -

          Patch attached adds default implementations for AbstractFileSystem so new file system implementations don't need to override the various symlink APIs, just as existing FileSystem based ones don't because DelegateToFileSystem provides default implementations. This allows the common patch to be checked in independently of the HDFS one. I'm running test-core on a latest trunk HDFS repo against a latest common repo with this patch applied to make sure it can in fact go in independently. So far so good.

          Show
          Eli Collins added a comment - Patch attached adds default implementations for AbstractFileSystem so new file system implementations don't need to override the various symlink APIs, just as existing FileSystem based ones don't because DelegateToFileSystem provides default implementations. This allows the common patch to be checked in independently of the HDFS one. I'm running test-core on a latest trunk HDFS repo against a latest common repo with this patch applied to make sure it can in fact go in independently. So far so good.
          Hide
          Eli Collins added a comment -

          Forgot to mention, patches 34 through 36 addressed Sanjay's feedback in the previous comment.

          Show
          Eli Collins added a comment - Forgot to mention, patches 34 through 36 addressed Sanjay's feedback in the previous comment.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12435010/symlink37-common.patch
          against trunk revision 906388.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 14 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12435010/symlink37-common.patch against trunk revision 906388. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 14 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/console This message is automatically generated.
          Hide
          Sanjay Radia added a comment -

          My final review comments on patch 37.
          Thanks for addressing my previous comments; Thanks for declaring the exception; however you missed/have mistakes on a few.

          • Javadoc : FileContext#createSymLink
            • you added more details in Javadoc but not the semantics of the operations
            • suggestion add:
              • The following operations follow (ie resolve through) the symlinks: open, set/getWorkingDir, setReplication, setPermission,
                setOwner, setTimes, get/setFileChecksum, getFileStatus, isDirectory, isFile, listStatus, getFileBlockLocations, getFsStatus.
              • The following operations follow intermediate symlinks in the path but not the final leaf symlink:
                create/mkdir the leaf should not exit.
                delete, deleteOnExit removes the leaf symlink
          • UnresolvedLinkException
            • AbstractFileSystem#getServerDefaults should not throw the UnresolvedLinkException
            • AbstractFileSystem#getLinkTarget - should this throw UnresolvedLinkException?
            • Why does Filterfs throw UnresolvedLinkException on only some of its methods with a path name arg and not all.
          • RawLocalFs and symlinks – I missed this in my earlier comments.
            RawLocalFs is suppose to support only volume root relative and dot relative symlinks (links to other file systems
            are not supported). Noticed that you strip out the scheme - instead throw an exception if the link is to a different file system.
            (You will also have to fix the test that takes advantage of this).

          Some comments on tests in my next comment.

          Show
          Sanjay Radia added a comment - My final review comments on patch 37. Thanks for addressing my previous comments; Thanks for declaring the exception; however you missed/have mistakes on a few. Javadoc : FileContext#createSymLink you added more details in Javadoc but not the semantics of the operations suggestion add: The following operations follow (ie resolve through) the symlinks: open, set/getWorkingDir, setReplication, setPermission, setOwner, setTimes, get/setFileChecksum, getFileStatus, isDirectory, isFile, listStatus, getFileBlockLocations, getFsStatus. The following operations follow intermediate symlinks in the path but not the final leaf symlink: create/mkdir the leaf should not exit. delete, deleteOnExit removes the leaf symlink UnresolvedLinkException AbstractFileSystem#getServerDefaults should not throw the UnresolvedLinkException AbstractFileSystem#getLinkTarget - should this throw UnresolvedLinkException? Why does Filterfs throw UnresolvedLinkException on only some of its methods with a path name arg and not all. RawLocalFs and symlinks – I missed this in my earlier comments. RawLocalFs is suppose to support only volume root relative and dot relative symlinks (links to other file systems are not supported). Noticed that you strip out the scheme - instead throw an exception if the link is to a different file system. (You will also have to fix the test that takes advantage of this). Some comments on tests in my next comment.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Could you also add some javadoc for T and the public methods in FSLinkResolver?

          Show
          Tsz Wo Nicholas Sze added a comment - Could you also add some javadoc for T and the public methods in FSLinkResolver?
          Hide
          Sanjay Radia added a comment -

          Comments on the tests for patch 37.
          Thanks for addressing my comments.

          • testLocalFSFileContextSymlink#testGetLinkStatusPartQualTarget - you create a link "hdfs://host:1000/dir/file". This link should fail on local fs. (see
            my previous comment).
          • Move the setPermissions and setTimes tests from HDFS to Common (you can do this in a separate jira if you prefer, though you might find it
            easier to do in this patch.)
          • File a jira to use the test library (your issue with the test library and eclipse noted). We need to fix the test library to address the eclipse issue and
            then change the symlink test. The other fileContext tests use the test library.
          Show
          Sanjay Radia added a comment - Comments on the tests for patch 37. Thanks for addressing my comments. testLocalFSFileContextSymlink#testGetLinkStatusPartQualTarget - you create a link "hdfs://host:1000/dir/file". This link should fail on local fs. (see my previous comment). Move the setPermissions and setTimes tests from HDFS to Common (you can do this in a separate jira if you prefer, though you might find it easier to do in this patch.) File a jira to use the test library (your issue with the test library and eclipse noted). We need to fix the test library to address the eclipse issue and then change the symlink test. The other fileContext tests use the test library.
          Hide
          Eli Collins added a comment -

          The attached patch incorporates the feedback from you and Nicholas.

          • AFS#getLinkTarget will never thrown an UnresolvedLinkException because it resolves the first symlink in the path. See NameNode#getLinkTarget for details.
          • Left the permissions test as is and filed HDFS-971 to add symlinks to TestDFSPermission once it's converted to FileContext, and HADOOP-6561 to add symlink coverage to TestFcLocalFsPermission.
          • Moved the setTimes test to the base test class though the asserts are only checked for non-local file systems as FileSystem#setTimes is a nop and none of the local file system classes override it.
          • Filed HADOOP-6562 to make FileContextSymlinkBaseTest use FileContextTestHelper once HADOOP-5916 is resolved.

          Patch also merges with trunk. ant test-core passes.

          Show
          Eli Collins added a comment - The attached patch incorporates the feedback from you and Nicholas. AFS#getLinkTarget will never thrown an UnresolvedLinkException because it resolves the first symlink in the path. See NameNode#getLinkTarget for details. Left the permissions test as is and filed HDFS-971 to add symlinks to TestDFSPermission once it's converted to FileContext, and HADOOP-6561 to add symlink coverage to TestFcLocalFsPermission. Moved the setTimes test to the base test class though the asserts are only checked for non-local file systems as FileSystem#setTimes is a nop and none of the local file system classes override it. Filed HADOOP-6562 to make FileContextSymlinkBaseTest use FileContextTestHelper once HADOOP-5916 is resolved. Patch also merges with trunk. ant test-core passes.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The javadoc in FSLinkResolver looks good. Thanks, Eli.

          Show
          Tsz Wo Nicholas Sze added a comment - The javadoc in FSLinkResolver looks good. Thanks, Eli.
          Hide
          Eli Collins added a comment -

          Kicking Hudson, didn't seem to pick up on the submit patch from yesterday.

          Show
          Eli Collins added a comment - Kicking Hudson, didn't seem to pick up on the submit patch from yesterday.
          Hide
          Eli Collins added a comment -

          Attached updated patch that fixes RawLocalFs#readLink on OSX.

          Show
          Eli Collins added a comment - Attached updated patch that fixes RawLocalFs#readLink on OSX.
          Hide
          Eli Collins added a comment -

          Here are ant test-patch results in case Hudson continues to have problems.

          [exec] ======================================================================
          [exec] ======================================================================
          [exec]
          [exec]
          [exec] /home/eli/apache-ant-1.7.1/bin/ant -Dversion=PATCH-symlink39-common.patch -Djava5.home=/usr/lib/jvm/jdk1.5.0_21 -Dforrest.home=/home/eli/apache-forrest-0.8 -DHadoopPatchProcess= releaseaudit > /home/eli/tmp/patchReleaseAuditWarnings.txt 2>&1
          [exec]
          [exec]
          [exec] There appear to be 1 release audit warnings before the patch and 1 release audit warnings after applying the patch.
          [exec]
          [exec]
          [exec]
          [exec]
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 14 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          [exec]
          [exec]
          [exec]
          [exec]
          [exec] ======================================================================
          [exec] ======================================================================

          Show
          Eli Collins added a comment - Here are ant test-patch results in case Hudson continues to have problems. [exec] ====================================================================== [exec] ====================================================================== [exec] [exec] [exec] /home/eli/apache-ant-1.7.1/bin/ant -Dversion=PATCH-symlink39-common.patch -Djava5.home=/usr/lib/jvm/jdk1.5.0_21 -Dforrest.home=/home/eli/apache-forrest-0.8 -DHadoopPatchProcess= releaseaudit > /home/eli/tmp/patchReleaseAuditWarnings.txt 2>&1 [exec] [exec] [exec] There appear to be 1 release audit warnings before the patch and 1 release audit warnings after applying the patch. [exec] [exec] [exec] [exec] [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 14 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ======================================================================
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12435793/symlink39-common.patch
          against trunk revision 909806.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 14 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12435793/symlink39-common.patch against trunk revision 909806. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 14 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/console This message is automatically generated.
          Hide
          Sanjay Radia added a comment -

          +1

          Show
          Sanjay Radia added a comment - +1
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #169 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/169/)
          Adds Symbolic links to FileContext, AbstractFileSystem.
          It also adds a limited implementation for the local file system
          (RawLocalFs) that allows local symlinks. (Eli Collins via Sanjay Radia)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #169 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/169/ ) Adds Symbolic links to FileContext, AbstractFileSystem. It also adds a limited implementation for the local file system (RawLocalFs) that allows local symlinks. (Eli Collins via Sanjay Radia)
          Hide
          Sanjay Radia added a comment -

          Thanks Eli.

          Show
          Sanjay Radia added a comment - Thanks Eli.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #252 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/252/)
          Adds Symbolic links to FileContext, AbstractFileSystem.
          It also adds a limited implementation for the local file system
          (RawLocalFs) that allows local symlinks. (Eli Collins via Sanjay Radia)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #252 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/252/ ) Adds Symbolic links to FileContext, AbstractFileSystem. It also adds a limited implementation for the local file system (RawLocalFs) that allows local symlinks. (Eli Collins via Sanjay Radia)
          Hide
          Sanjay Radia added a comment -

          I missed that this breaks compatibility in a subtle way:
          Negation of FileStatus#isDir() is normally used to see if the target is a file. This will now break if the target happens to be symlink.

          I suggest we deprecate isDir(), remove isSymlink() and add new method that returns an enum.
          Alternatively we could deprecate isDir() and add isDirectory() and isFile();

          Show
          Sanjay Radia added a comment - I missed that this breaks compatibility in a subtle way: Negation of FileStatus#isDir() is normally used to see if the target is a file. This will now break if the target happens to be symlink. I suggest we deprecate isDir(), remove isSymlink() and add new method that returns an enum. Alternatively we could deprecate isDir() and add isDirectory() and isFile();
          Hide
          Eli Collins added a comment -

          Nice catch Sanjay.

          Alternatively we could deprecate isDir() and add isDirectory() and isFile();

          This sounds good to me. I'll send out a patch.

          Show
          Eli Collins added a comment - Nice catch Sanjay. Alternatively we could deprecate isDir() and add isDirectory() and isFile(); This sounds good to me. I'll send out a patch.
          Hide
          dhruba borthakur added a comment -

          +1 for adding isDirectory() and isFile()

          Show
          dhruba borthakur added a comment - +1 for adding isDirectory() and isFile()
          Hide
          Zlatin Balevsky added a comment -

          +1 for adding isDirectory() and isFile()

          +1 here too.. makes it easier to add new types of INodes that are neither files nor directories

          Show
          Zlatin Balevsky added a comment - +1 for adding isDirectory() and isFile() +1 here too.. makes it easier to add new types of INodes that are neither files nor directories
          Hide
          Eli Collins added a comment -

          Hey Zlatin,

          The intent for FileStatus (and FileContext) is to expose a least common denominator API for all file systems that Hadoop uses (HDFS, Local, S3, etc) so file system specific stuff like dev, proc, etc are all things we'd like to keep out of the API.

          Thanks,
          Eli

          Show
          Eli Collins added a comment - Hey Zlatin, The intent for FileStatus (and FileContext) is to expose a least common denominator API for all file systems that Hadoop uses (HDFS, Local, S3, etc) so file system specific stuff like dev, proc, etc are all things we'd like to keep out of the API. Thanks, Eli
          Hide
          Eli Collins added a comment -

          Filed HADOOP-6585 for the new API and changing common uses of isDir(), filed HDFS-995 for replacing HDFS uses of isDir().

          Show
          Eli Collins added a comment - Filed HADOOP-6585 for the new API and changing common uses of isDir(), filed HDFS-995 for replacing HDFS uses of isDir().
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #201 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/201/)
          HDFS-245. Adds a symlink implementation to HDFS. This complements the new symlink feature added in
          (Eli Collins via Sanjay Radia)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #201 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/201/ ) HDFS-245 . Adds a symlink implementation to HDFS. This complements the new symlink feature added in (Eli Collins via Sanjay Radia)
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #146 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/146/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #146 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/146/ )
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #302 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/302/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #302 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/302/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/ )

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development