Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13885

Implement getLinkTarget for ViewFileSystem

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha1
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: viewfs
    • Labels:
      None

      Description

      ViewFileSystem doesn't override FileSystem#getLinkTarget(). So, when view filesystem is used to resolve the symbolic links, the default FileSystem implementation throws UnsupportedOperationException.

      The proposal is to define getLinkTarget() for ViewFileSystem and invoke the target FileSystem for resolving the symbolic links. Path thus returned is preferred to be a viewfs qualified path, so that it can be used again on the ViewFileSystem handle.

      1. HADOOP-13885.01.patch
        5 kB
        Manoj Govindassamy
      2. HADOOP-13885.02.patch
        6 kB
        Manoj Govindassamy

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11094 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11094/)
        HADOOP-13885. Implement getLinkTarget for ViewFileSystem. Contributed by (wang: rev 511d39e0740f36bf937e7bcf974e1050f0e7c1e0)

        • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
        • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ChRootedFileSystem.java
        • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11094 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11094/ ) HADOOP-13885 . Implement getLinkTarget for ViewFileSystem. Contributed by (wang: rev 511d39e0740f36bf937e7bcf974e1050f0e7c1e0) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ChRootedFileSystem.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
        Hide
        andrew.wang Andrew Wang added a comment -

        LGTM +1, thanks for the contribution Manoj! I've committed this to trunk.

        Show
        andrew.wang Andrew Wang added a comment - LGTM +1, thanks for the contribution Manoj! I've committed this to trunk.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 14s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 12m 49s trunk passed
        +1 compile 9m 44s trunk passed
        +1 checkstyle 0m 30s trunk passed
        +1 mvnsite 1m 2s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 26s trunk passed
        +1 javadoc 0m 48s trunk passed
        +1 mvninstall 0m 39s the patch passed
        +1 compile 9m 20s the patch passed
        +1 javac 9m 20s the patch passed
        +1 checkstyle 0m 29s the patch passed
        +1 mvnsite 1m 0s the patch passed
        +1 mvneclipse 0m 18s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 34s the patch passed
        +1 javadoc 0m 48s the patch passed
        +1 unit 8m 47s hadoop-common in the patch passed.
        +1 asflicense 0m 32s The patch does not generate ASF License warnings.
        52m 2s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue HADOOP-13885
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846108/HADOOP-13885.02.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux f35eb02f23b8 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 71a4acf
        Default Java 1.8.0_111
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11388/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11388/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 12m 49s trunk passed +1 compile 9m 44s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 2s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 48s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 9m 20s the patch passed +1 javac 9m 20s the patch passed +1 checkstyle 0m 29s the patch passed +1 mvnsite 1m 0s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 34s the patch passed +1 javadoc 0m 48s the patch passed +1 unit 8m 47s hadoop-common in the patch passed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 52m 2s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13885 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846108/HADOOP-13885.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f35eb02f23b8 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 71a4acf Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11388/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11388/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        manojg Manoj Govindassamy added a comment -

        Thanks for the review Andrew Wang. Can you please take a look at the updated patch ?
        Attached v02 patch to address the following

        • Made ViewFileSystem#getLinkTarget throw NotInMountPointException when internal mount point resolve fails and throw other exceptions including FNFE from target filesystem as is.
        • Updated ViewFileSystemBaseTest to verify both normal and relative symbolic links, and added assume to skip irrelevant tests.
        Show
        manojg Manoj Govindassamy added a comment - Thanks for the review Andrew Wang . Can you please take a look at the updated patch ? Attached v02 patch to address the following Made ViewFileSystem#getLinkTarget throw NotInMountPointException when internal mount point resolve fails and throw other exceptions including FNFE from target filesystem as is. Updated ViewFileSystemBaseTest to verify both normal and relative symbolic links, and added assume to skip irrelevant tests.
        Hide
        manojg Manoj Govindassamy added a comment -
        • sure, will make getLinkTarget() return resolved path as is.
        • There are few other places like getDefaultBlocksize(), getDefaultReplication(), getServerDefaults(), getTrashRoot() which has try block surrounding both viewfs internal resolve and the target filesystem API call, catch FNFE/Exception and re-throw as NotInMountpointException. But all these places seem to be fine as the target filesystem calls in these APIs don't throw any FNFE. So, we will not be masking any FNFE from target filesystem in these APIs. Will anyway take a closer look and fix the needed ones as a separate jira.
        Show
        manojg Manoj Govindassamy added a comment - sure, will make getLinkTarget() return resolved path as is. There are few other places like getDefaultBlocksize(), getDefaultReplication(), getServerDefaults(), getTrashRoot() which has try block surrounding both viewfs internal resolve and the target filesystem API call, catch FNFE/Exception and re-throw as NotInMountpointException. But all these places seem to be fine as the target filesystem calls in these APIs don't throw any FNFE. So, we will not be masking any FNFE from target filesystem in these APIs. Will anyway take a closer look and fix the needed ones as a separate jira.
        Hide
        andrew.wang Andrew Wang added a comment -

        Hi Manoj,

        I was following the getFileStatus() to model, to qualify returned paths. Let me know if this raw target filesystem resolved path is ok and no more viewfs qualification is needed.

        I'm pretty sure link targets are supposed to be returned as is, per the FileContext documentation. It's similar to man 2 readlink, which returns the contents directly.

        In the attached patch v01, try block surrounds both Case 1 and Case 2 together. I will fix this in the next patch revision so that only Case 1 returns NotInMountPointException and Case 2 returns FNFE.

        SGTM! Are there other places too where this should be changed?

        Show
        andrew.wang Andrew Wang added a comment - Hi Manoj, I was following the getFileStatus() to model, to qualify returned paths. Let me know if this raw target filesystem resolved path is ok and no more viewfs qualification is needed. I'm pretty sure link targets are supposed to be returned as is, per the FileContext documentation. It's similar to man 2 readlink , which returns the contents directly. In the attached patch v01, try block surrounds both Case 1 and Case 2 together. I will fix this in the next patch revision so that only Case 1 returns NotInMountPointException and Case 2 returns FNFE. SGTM! Are there other places too where this should be changed?
        Hide
        manojg Manoj Govindassamy added a comment -

        The goal is to make ViewFileSystem support getLinkTarget() to resolve the target filesystem paths, so that users of this file system can resolve their paths based on viewfs mounts.

        1. Now coming to whether the resolved path has to be returned as is OR make the resolved path qualified it on top of viewfs mount path, ViewFileSystem already does this for "ls", "du", getHomeDirectory(), etc., Yes, they could further be links to another file system or relative one, but all the path qualification does is it replaces the target filesystem prefix path with the viewfs mount path (as defined in the config)

        # hdfs dfs -ls  /
        -r-xr-xr-x   - manoj staff          0 2017-01-05 17:32 /nn0
        -r-xr-xr-x   - manoj staff          0 2017-01-05 17:32 /nn1
        -r-xr-xr-x   - manoj staff          0 2017-01-05 17:32 /nn2
        -r-xr-xr-x   - manoj staff          0 2017-01-05 17:32 /nn3
        
        # hdfs dfs -ls -R /nn0/
        drwxr-xr-x   - manoj supergroup          0 2017-01-05 17:23 /nn0/user
        drwxr-xr-x   - manoj supergroup          0 2017-01-05 17:23 /nn0/user/manoj
        
        

        Say, we have the following symbolic link in the target file system.
        hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/debug.link ==> hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/debug.log

        Now, if the same target file system "hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/" is mounted on "/nn1/", then

        With resolved path qualified to viewfs mount path:
        /nn1/debug.link => /nn1/debug.log

        With resolved path non qualified:
        /nn1/debug.link => hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/debug.log

        I was following the getFileStatus() to model, to qualify returned paths. Let me know if this raw target filesystem resolved path is ok and no more viewfs qualification is needed.

        2. We can potentially get FileNotFoundExceptions from 2 places.

        • Case 1: From ViewFileSystem internal InodeTree resolve which traverses the internal mount tree leading to the final InodeLink which links the target filesystem, when the given path doesn't map to a proper mountpoint configured.
        • Case 2: From the TargetFileSystem getLinkTarget() when the given path is missing.

        So, we want to return NotInMountPointException for the Case 1 above. Where as for the Case 2, we want to return the FNFE as is.

        In the attached patch v01, try block surrounds both Case 1 and Case 2 together. I will fix this in the next patch revision so that only Case 1 returns NotInMountPointException and Case 2 returns FNFE.

        Andrew Wang, let me know your views on (1) and I will do necessary changes and upload the next patch revision. Thanks for the review.

        Show
        manojg Manoj Govindassamy added a comment - The goal is to make ViewFileSystem support getLinkTarget() to resolve the target filesystem paths, so that users of this file system can resolve their paths based on viewfs mounts. 1. Now coming to whether the resolved path has to be returned as is OR make the resolved path qualified it on top of viewfs mount path, ViewFileSystem already does this for "ls", "du", getHomeDirectory(), etc., Yes, they could further be links to another file system or relative one, but all the path qualification does is it replaces the target filesystem prefix path with the viewfs mount path (as defined in the config) # hdfs dfs -ls / -r-xr-xr-x - manoj staff 0 2017-01-05 17:32 /nn0 -r-xr-xr-x - manoj staff 0 2017-01-05 17:32 /nn1 -r-xr-xr-x - manoj staff 0 2017-01-05 17:32 /nn2 -r-xr-xr-x - manoj staff 0 2017-01-05 17:32 /nn3 # hdfs dfs -ls -R /nn0/ drwxr-xr-x - manoj supergroup 0 2017-01-05 17:23 /nn0/user drwxr-xr-x - manoj supergroup 0 2017-01-05 17:23 /nn0/user/manoj Say, we have the following symbolic link in the target file system. hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/debug.link ==> hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/debug.log Now, if the same target file system "hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/" is mounted on "/nn1/", then With resolved path qualified to viewfs mount path: /nn1/debug.link => /nn1/debug.log With resolved path non qualified: /nn1/debug.link => hdfs://localhost:52217/tmp/TestViewFileSystemHdfs/debug.log I was following the getFileStatus() to model, to qualify returned paths. Let me know if this raw target filesystem resolved path is ok and no more viewfs qualification is needed. 2. We can potentially get FileNotFoundExceptions from 2 places. Case 1: From ViewFileSystem internal InodeTree resolve which traverses the internal mount tree leading to the final InodeLink which links the target filesystem, when the given path doesn't map to a proper mountpoint configured. Case 2: From the TargetFileSystem getLinkTarget() when the given path is missing. So, we want to return NotInMountPointException for the Case 1 above. Where as for the Case 2, we want to return the FNFE as is. In the attached patch v01, try block surrounds both Case 1 and Case 2 together. I will fix this in the next patch revision so that only Case 1 returns NotInMountPointException and Case 2 returns FNFE. Andrew Wang , let me know your views on (1) and I will do necessary changes and upload the next patch revision. Thanks for the review.
        Hide
        andrew.wang Andrew Wang added a comment -

        Hi Manoj, thanks for working on this. IIUC getLinkTarget is supposed to return the link untouched, per this description in FileContext:

           * Returns the target of the given symbolic link as it was specified
           * when the link was created.  Links in the path leading up to the
           * final path component are resolved transparently.
        

        The implementation of getLinkTarget looks like it munges the path in some way. Note that symlinks can point to other filesystems, and can also be relative. The true location of the target depends on the mounts, and it's on the link resolver (not getLinkTarget) to further resolve links. I don't think we implemented symlink support for ViewFileSystem though, so this is somewhat moot.

        Sorry for not seeing these before, but I'm confused about how here (and elsewhere in VFS) we catch exceptions and turn them into a NotInMountPointException. Don't users expect an FNF if the link doesn't exist? A lot of methods just delegate directly to the targetFileSystem instead.

        In the test, we can also use assume from JUnit to skip if the test isn't applicable. It'd also be good to test relative and cross FS symlinks, for some additional completeness.

        Show
        andrew.wang Andrew Wang added a comment - Hi Manoj, thanks for working on this. IIUC getLinkTarget is supposed to return the link untouched, per this description in FileContext: * Returns the target of the given symbolic link as it was specified * when the link was created. Links in the path leading up to the * final path component are resolved transparently. The implementation of getLinkTarget looks like it munges the path in some way. Note that symlinks can point to other filesystems, and can also be relative. The true location of the target depends on the mounts, and it's on the link resolver (not getLinkTarget) to further resolve links. I don't think we implemented symlink support for ViewFileSystem though, so this is somewhat moot. Sorry for not seeing these before, but I'm confused about how here (and elsewhere in VFS) we catch exceptions and turn them into a NotInMountPointException. Don't users expect an FNF if the link doesn't exist? A lot of methods just delegate directly to the targetFileSystem instead. In the test, we can also use assume from JUnit to skip if the test isn't applicable. It'd also be good to test relative and cross FS symlinks, for some additional completeness.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 10s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 44s trunk passed
        +1 compile 9m 31s trunk passed
        +1 checkstyle 0m 29s trunk passed
        +1 mvnsite 1m 1s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 24s trunk passed
        +1 javadoc 0m 52s trunk passed
        +1 mvninstall 0m 44s the patch passed
        +1 compile 10m 45s the patch passed
        +1 javac 10m 45s the patch passed
        +1 checkstyle 0m 29s the patch passed
        +1 mvnsite 1m 5s the patch passed
        +1 mvneclipse 0m 17s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 53s the patch passed
        +1 javadoc 0m 52s the patch passed
        +1 unit 9m 11s hadoop-common in the patch passed.
        +1 asflicense 0m 31s The patch does not generate ASF License warnings.
        48m 7s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue HADOOP-13885
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842621/HADOOP-13885.01.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 9cd0ca421101 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 5bd7dec
        Default Java 1.8.0_111
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11237/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11237/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 44s trunk passed +1 compile 9m 31s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 24s trunk passed +1 javadoc 0m 52s trunk passed +1 mvninstall 0m 44s the patch passed +1 compile 10m 45s the patch passed +1 javac 10m 45s the patch passed +1 checkstyle 0m 29s the patch passed +1 mvnsite 1m 5s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 53s the patch passed +1 javadoc 0m 52s the patch passed +1 unit 9m 11s hadoop-common in the patch passed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 48m 7s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13885 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842621/HADOOP-13885.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9cd0ca421101 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5bd7dec Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11237/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11237/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        manojg Manoj Govindassamy added a comment -

        Attached a patch which defines getLinkTarget() for ViewFileSystem along with testcases.
        Andrew Wang, please take a look at the patch.

        Show
        manojg Manoj Govindassamy added a comment - Attached a patch which defines getLinkTarget() for ViewFileSystem along with testcases. Andrew Wang , please take a look at the patch.

          People

          • Assignee:
            manojg Manoj Govindassamy
            Reporter:
            manojg Manoj Govindassamy
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development