Details

    • New Feature
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • None
    • None
    • None
    • Incompatible change, Reviewed
    • 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.

      Attachments

        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

            eli Eli Collins added a comment -

            Latest patch attached.

            eli Eli Collins added a comment - Latest patch attached.
            cutting 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.

            cutting 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.
            eli 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).

            eli 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).
            cutting Doug Cutting added a comment -

            No, I meant LocalFs. Sounds good.

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

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

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

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

            eli Eli Collins added a comment - Attached the latest patch, includes a symlink implementation for LocalFs and more tests.
            eli 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?

            eli 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?
            cutting 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?

            cutting 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?
            omalley 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.

            omalley 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.
            eli Eli Collins added a comment -

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

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

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

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

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

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

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

            eli Eli Collins added a comment - Yup, that's intentional. I've linked the issues.
            eli 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.

            eli 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.
            eli Eli Collins added a comment -

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

            eli Eli Collins added a comment - Latest patch implements the semantics and adds tests according to the previous update.

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

            dhruba Dhruba Borthakur added a comment - Your latest desciption of relative/absolute/fullyqualified link targets make sense. +1.
            eli 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)

            eli 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)
            eli 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.
            eli 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.
            eli 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.

            eli 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.
            hadoopqa 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.

            hadoopqa 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.
            eli 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.

            eli 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.
            eli Eli Collins added a comment -

            Minor update to address javadoc warnings and checkstyle suggestions.

            eli Eli Collins added a comment - Minor update to address javadoc warnings and checkstyle suggestions.
            hadoopqa 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.

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

            hadoopqa 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.
            eli 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.

            eli 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.
            eli Eli Collins added a comment -

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

            eli Eli Collins added a comment - Same patch different name. Hudson used the old copy of the previous patch.
            hadoopqa 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.

            hadoopqa 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.
            sanjay.radia 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.

            sanjay.radia 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.
            eli 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.

            eli 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.
            hadoopqa 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.

            hadoopqa 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.
            tlipcon 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.

            tlipcon 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.
            eli 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.

            eli 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.
            hadoopqa 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.

            hadoopqa 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.
            eli 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.

            eli 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.
            hadoopqa 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.

            hadoopqa 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.
            sanjay.radia 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.
            sanjay.radia 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.
            sanjay.radia 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.
            sanjay.radia 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.

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

            aw 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.]
            eli 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.

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

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

            dhruba 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.
            eli 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.

            eli 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.
            hadoopqa 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.

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

            Is this patch ready to commit?

            hammer Jeff Hammerbacher added a comment - Is this patch ready to commit?
            eli 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?

            eli 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?
            eli Eli Collins added a comment -

            Patch with latest common changes to match HDFS-245.

            eli Eli Collins added a comment - Patch with latest common changes to match HDFS-245 .
            hadoopqa 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.

            hadoopqa 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.
            eli Eli Collins added a comment -

            Patch with latest common changes to match HDFS-245.

            eli Eli Collins added a comment - Patch with latest common changes to match HDFS-245 .
            hadoopqa 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.

            hadoopqa 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.
            sanjay.radia 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.
            sanjay.radia 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.
            eli 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.

            eli 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.
            eli Eli Collins added a comment -

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

            eli Eli Collins added a comment - Forgot to mention, patches 34 through 36 addressed Sanjay's feedback in the previous comment.
            hadoopqa 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.

            hadoopqa 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.
            sanjay.radia 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.

            sanjay.radia 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.
            szetszwo Tsz-wo Sze added a comment -

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

            szetszwo Tsz-wo Sze added a comment - Could you also add some javadoc for T and the public methods in FSLinkResolver?
            sanjay.radia 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.
            sanjay.radia 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.
            eli 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.

            eli 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.
            szetszwo Tsz-wo Sze added a comment -

            The javadoc in FSLinkResolver looks good. Thanks, Eli.

            szetszwo Tsz-wo Sze added a comment - The javadoc in FSLinkResolver looks good. Thanks, Eli.
            eli Eli Collins added a comment -

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

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

            Attached updated patch that fixes RawLocalFs#readLink on OSX.

            eli Eli Collins added a comment - Attached updated patch that fixes RawLocalFs#readLink on OSX.
            eli 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] ======================================================================

            eli 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] ======================================================================
            hadoopqa 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.

            hadoopqa 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.
            sanjay.radia Sanjay Radia added a comment -

            +1

            sanjay.radia Sanjay Radia added a comment - +1
            hudson 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)

            hudson 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)
            sanjay.radia Sanjay Radia added a comment -

            Thanks Eli.

            sanjay.radia Sanjay Radia added a comment - Thanks Eli.
            hudson 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)

            hudson 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)
            sanjay.radia 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();

            sanjay.radia 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();
            eli 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.

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

            +1 for adding isDirectory() and isFile()

            dhruba Dhruba Borthakur added a comment - +1 for adding isDirectory() and isFile()

            +1 for adding isDirectory() and isFile()

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

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

            eli 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
            eli 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().

            eli 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().
            hudson 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)

            hudson 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)
            hudson 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/)

            hudson 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/ )
            hudson 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/)

            hudson 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/ )
            hudson Hudson added a comment -

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

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

            People

              eli Eli Collins
              eli Eli Collins
              Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: