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

Remove confusing comment in Path#isAbsolute()

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The method is checking for absolute path correctly. When scheme and authority are present, the path is absolute. So the following comment needs to be removed.

        /**
         * There is some ambiguity here. An absolute path is a slash
         * relative name without a scheme or an authority.
         * So either this method was incorrectly named or its
         * implementation is incorrect. This method returns true
         * even if there is a scheme and authority.
         */
      
      1. HADOOP-8174.txt
        0.9 kB
        Suresh Srinivas
      2. HADOOP-8174.patch
        1 kB
        Harsh J

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2143/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2143/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #185 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/185/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #185 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/185/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2125 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2125/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2125 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2125/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #195 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/195/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #195 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/195/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk-Java8 #196 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/196/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk-Java8 #196 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/196/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #927 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/927/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #927 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/927/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7829 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7829/)
        HADOOP-8174. Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7829 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7829/ ) HADOOP-8174 . Remove confusing comment in Path#isAbsolute() (Contributed by Suresh Srinivas) (vinayakumarb: rev 0daa5ada68db483275aaa7f2ed9a2b5eaf5bb9bd) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
        Hide
        vinayrpet Vinayakumar B added a comment -

        Committed to trunk and branch-2

        Show
        vinayrpet Vinayakumar B added a comment - Committed to trunk and branch-2
        Hide
        vinayrpet Vinayakumar B added a comment -

        I think it's before our supporting Windows.

        I think there is no problem here. because hadoop implementation uses '/' as Path.SEPARATOR. and its URI standard.
        Patch looks good +1

        Show
        vinayrpet Vinayakumar B added a comment - I think it's before our supporting Windows. I think there is no problem here. because hadoop implementation uses '/' as Path.SEPARATOR. and its URI standard. Patch looks good +1
        Hide
        ozawa Tsuyoshi Ozawa added a comment -

        Harsh J I took a look at the patch, and I think it's before our supporting Windows. Precisely, we can say "slash for Unix OS, back slack for Windows". What do you think?

        Show
        ozawa Tsuyoshi Ozawa added a comment - Harsh J I took a look at the patch, and I think it's before our supporting Windows. Precisely, we can say "slash for Unix OS, back slack for Windows". What do you think?
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 14m 30s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 javac 7m 28s There were no new javac warning messages.
        +1 javadoc 9m 33s There were no new javadoc warning messages.
        +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
        +1 checkstyle 1m 3s There were no new checkstyle issues.
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 32s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 1m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
        +1 common tests 22m 21s Tests passed in hadoop-common.
            59m 5s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12546157/HADOOP-8174.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / f1a152c
        hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6387/artifact/patchprocess/testrun_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6387/testReport/
        Java 1.7.0_55
        uname Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6387/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 30s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 28s There were no new javac warning messages. +1 javadoc 9m 33s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 1m 3s There were no new checkstyle issues. +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 32s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 common tests 22m 21s Tests passed in hadoop-common.     59m 5s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12546157/HADOOP-8174.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / f1a152c hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6387/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6387/testReport/ Java 1.7.0_55 uname Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6387/console This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 14m 32s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 javac 7m 29s There were no new javac warning messages.
        +1 javadoc 9m 33s There were no new javadoc warning messages.
        +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
        +1 checkstyle 1m 3s There were no new checkstyle issues.
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 34s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 1m 39s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
        +1 common tests 23m 12s Tests passed in hadoop-common.
            60m 3s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12546157/HADOOP-8174.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / f1a152c
        hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6382/artifact/patchprocess/testrun_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6382/testReport/
        Java 1.7.0_55
        uname Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6382/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 32s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 29s There were no new javac warning messages. +1 javadoc 9m 33s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 1m 3s There were no new checkstyle issues. +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 34s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 39s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 common tests 23m 12s Tests passed in hadoop-common.     60m 3s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12546157/HADOOP-8174.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / f1a152c hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6382/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6382/testReport/ Java 1.7.0_55 uname Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6382/console This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12546157/HADOOP-8174.patch
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

        org.apache.hadoop.ha.TestZKFailoverController
        org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1496//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1496//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12546157/HADOOP-8174.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1496//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1496//console This message is automatically generated.
        Hide
        qwertymaniac Harsh J added a comment -

        Rebased Suresh's patch for trunk with Eli's second comment addressed.

        Show
        qwertymaniac Harsh J added a comment - Rebased Suresh's patch for trunk with Eli's second comment addressed.
        Hide
        eli2 Eli Collins added a comment -

        @Suresh, looks good. How about removing the comment "A path string is absolute if it begins with a slash" at the top of the file so we just have one place that defines what absolute means.

        @Sanjay, unfortunately URI is not an implementation detail of Path, the Path constructor takes a java.net.URI object, has a toUri method that returns one that is used extensively. The code uses Paths and URIs interchangably so the URI semantics are leaked all over the Hadoop code base.

        Show
        eli2 Eli Collins added a comment - @Suresh, looks good. How about removing the comment "A path string is absolute if it begins with a slash" at the top of the file so we just have one place that defines what absolute means. @Sanjay, unfortunately URI is not an implementation detail of Path, the Path constructor takes a java.net.URI object, has a toUri method that returns one that is used extensively. The code uses Paths and URIs interchangably so the URI semantics are leaked all over the Hadoop code base.
        Hide
        sureshms Suresh Srinivas added a comment -

        isAbsolute() is a method on Path and so the method should not be attributed to URI, even if Path wraps URI, which is implementation detail.

        Now I get what the problem is, there is a comment:

        {format}
        /** True if the path component of this URI is absolute. */{format}

        Hence the comment following it. I will fix the comment.

        Show
        sureshms Suresh Srinivas added a comment - isAbsolute() is a method on Path and so the method should not be attributed to URI, even if Path wraps URI, which is implementation detail. Now I get what the problem is, there is a comment: {format} /** True if the path component of this URI is absolute. */{format} Hence the comment following it. I will fix the comment.
        Hide
        eli2 Eli Collins added a comment -

        The confusion is because there are two separate perspectives:

        1. Whether a path is absolute (ie the reference is non-relative, starts with the root). Ie /foo is an absolute path.
        2. Whether a URI is absolute (ie URI#isAbsolute where "A URI is absolute if, and only if, it has a scheme component"). Ie "/foo" is not absolute URI, but file:///foo is.

        Because Hadoop Paths are represented by, and are just thin wrappers around, URIs (which is goofy!) there is some ambiguity.

        Given that the intent of Path#isAbsolute is #1 (the class-level javadoc states "A path string is absolute if it begins with a slash.") I think we should update the Path#isAbsolute javadoc to indicate this. It's also worth putting the above discussion in a class-level comment since it's not clear that we intend Path to be a file system path in most cases.

        Show
        eli2 Eli Collins added a comment - The confusion is because there are two separate perspectives: Whether a path is absolute (ie the reference is non-relative, starts with the root). Ie /foo is an absolute path. Whether a URI is absolute (ie URI#isAbsolute where "A URI is absolute if, and only if, it has a scheme component"). Ie "/foo" is not absolute URI, but file:///foo is. Because Hadoop Paths are represented by, and are just thin wrappers around, URIs (which is goofy!) there is some ambiguity. Given that the intent of Path#isAbsolute is #1 (the class-level javadoc states "A path string is absolute if it begins with a slash.") I think we should update the Path#isAbsolute javadoc to indicate this. It's also worth putting the above discussion in a class-level comment since it's not clear that we intend Path to be a file system path in most cases.

          People

          • Assignee:
            sureshms Suresh Srinivas
            Reporter:
            sureshms Suresh Srinivas
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development