Hadoop Common
  1. Hadoop Common
  2. HADOOP-7360

FsShell does not preserve relative paths with globs

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0, 0.24.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      FsShell currently preserves relative paths that do not contain globs. Unfortunately the method fs.globStatus() is fully qualifying all returned paths. This is causing inconsistent display of paths.

      1. HADOOP-7360.patch
        18 kB
        Daryn Sharp
      2. HADOOP-7360.txt
        29 kB
        Kihwal Lee
      3. HADOOP-7360.txt
        30 kB
        Kihwal Lee
      4. HADOOP-7360.txt
        28 kB
        Kihwal Lee
      5. HADOOP-7360-2.patch
        24 kB
        Daryn Sharp
      6. HADOOP-7360-3.patch
        25 kB
        Daryn Sharp
      7. HADOOP-7360-4.patch
        25 kB
        Daryn Sharp

        Issue Links

          Activity

          Gavin made changes -
          Link This issue is depended upon by HDFS-2038 [ HDFS-2038 ]
          Gavin made changes -
          Link This issue blocks HDFS-2038 [ HDFS-2038 ]
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #884 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/884/)
          TestHDFSCLI is failing due to HADOOP-7360. HDFS-2038 is going to fix it. Disable it for the moment.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #884 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/884/ ) TestHDFSCLI is failing due to HADOOP-7360 . HDFS-2038 is going to fix it. Disable it for the moment. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #850 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/850/)
          TestHDFSCLI is failing due to HADOOP-7360. HDFS-2038 is going to fix it. Disable it for the moment.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #850 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/850/ ) TestHDFSCLI is failing due to HADOOP-7360 . HDFS-2038 is going to fix it. Disable it for the moment. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1239 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1239/)
          TestHDFSCLI is failing due to HADOOP-7360. HDFS-2038 is going to fix it. Disable it for the moment.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1239 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1239/ ) TestHDFSCLI is failing due to HADOOP-7360 . HDFS-2038 is going to fix it. Disable it for the moment. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1214 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1214/)
          TestHDFSCLI is failing due to HADOOP-7360. HDFS-2038 is going to fix it. Disable it for the moment.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1214 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1214/ ) TestHDFSCLI is failing due to HADOOP-7360 . HDFS-2038 is going to fix it. Disable it for the moment. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1290 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1290/)
          TestHDFSCLI is failing due to HADOOP-7360. HDFS-2038 is going to fix it. Disable it for the moment.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1290 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1290/ ) TestHDFSCLI is failing due to HADOOP-7360 . HDFS-2038 is going to fix it. Disable it for the moment. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1195754 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestHDFSCLI.java
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Hi Kihwal, thanks for fixing it. For the moment, let's disable the test. Otherwise, it is going to break every build.

          Show
          Tsz Wo Nicholas Sze added a comment - Hi Kihwal, thanks for fixing it. For the moment, let's disable the test. Otherwise, it is going to break every build.
          Hide
          Kihwal Lee added a comment -

          Sure thing. It's just that it will take a while to go through the xml file and fix the cases that are failing.

          Show
          Kihwal Lee added a comment - Sure thing. It's just that it will take a while to go through the xml file and fix the cases that are failing.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          TestHDFSCLI is failing. I believe you guys are going to fix it in HDFS-2038.

          Show
          Tsz Wo Nicholas Sze added a comment - TestHDFSCLI is failing. I believe you guys are going to fix it in HDFS-2038 .
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #846 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/846/)
          HADOOP-7360. Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #846 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/846/ ) HADOOP-7360 . Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #53 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/53/)
          svn merge -c 1190109 from trunk for HADOOP-7360.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #53 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/53/ ) svn merge -c 1190109 from trunk for HADOOP-7360 . szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1182 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1182/)
          HADOOP-7360. Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1182 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1182/ ) HADOOP-7360 . Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-0.23-Commit #90 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/90/)
          svn merge -c 1190109 from trunk for HADOOP-7360.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-0.23-Commit #90 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/90/ ) svn merge -c 1190109 from trunk for HADOOP-7360 . szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1259 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1259/)
          HADOOP-7360. Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1259 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1259/ ) HADOOP-7360 . Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Commit #90 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/90/)
          svn merge -c 1190109 from trunk for HADOOP-7360.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Commit #90 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/90/ ) svn merge -c 1190109 from trunk for HADOOP-7360 . szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Build #65 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/65/)
          svn merge -c 1190109 from trunk for HADOOP-7360.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Build #65 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/65/ ) svn merge -c 1190109 from trunk for HADOOP-7360 . szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #874 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/874/)
          HADOOP-7360. Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #874 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/874/ ) HADOOP-7360 . Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1198 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1198/)
          HADOOP-7360. Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1198 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1198/ ) HADOOP-7360 . Preserve relative paths that do not contain globs in FsShell. Contributed by Daryn Sharp and Kihwal Lee szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190109 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Commit #87 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/87/)
          svn merge -c 1190109 from trunk for HADOOP-7360.

          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Commit #87 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/87/ ) svn merge -c 1190109 from trunk for HADOOP-7360 . szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190111 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestPathData.java
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s 0.24.0 [ 12317652 ]
          Resolution Fixed [ 1 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          The findbugs warning is not related.

          I have committed this. Thanks, Daryn and Kihwal!

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good. The findbugs warning is not related. I have committed this. Thanks, Daryn and Kihwal!
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed unit tests in .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/336//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/336//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/336//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501166/HADOOP-7360.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/336//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/336//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/336//console This message is automatically generated.
          Tsz Wo Nicholas Sze made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Daryn Sharp added a comment -

          +1 Awesome! Thanks for taking it over the finish line.

          Show
          Daryn Sharp added a comment - +1 Awesome! Thanks for taking it over the finish line.
          Kihwal Lee made changes -
          Attachment HADOOP-7360.txt [ 12501166 ]
          Hide
          Kihwal Lee added a comment -
          1. Addressed the toString() issue using URI.getSchemeSpecificPart().
          1. Changed the private relativize() method to accept URI's instead of Path's to avoid extra conversions.
          Show
          Kihwal Lee added a comment - Addressed the toString() issue using URI.getSchemeSpecificPart() . Changed the private relativize() method to accept URI's instead of Path's to avoid extra conversions.
          Kihwal Lee made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Kihwal Lee added a comment -

          Well, actually URLDecoder will turn + into a space, while + is fine by URI.
          Daryn found out URI.getSchemeSpecificPart() can be used to get the properly decoded string after the scheme. So we just need to take care of scheme and append the scheme specific part. Will make the change and repost the patch.

          Show
          Kihwal Lee added a comment - Well, actually URLDecoder will turn + into a space, while + is fine by URI. Daryn found out URI.getSchemeSpecificPart() can be used to get the properly decoded string after the scheme. So we just need to take care of scheme and append the scheme specific part. Will make the change and repost the patch.
          Hide
          Kihwal Lee added a comment -

          URLDecoder.decode This is something I have been looking for. URI doesn't have anything equivalent to that.

          Show
          Kihwal Lee added a comment - URLDecoder.decode This is something I have been looking for. URI doesn't have anything equivalent to that.
          Hide
          Daryn Sharp added a comment -

          Glanced over it and the toString() made me ill... Why not: URLDecoder.decode(uri.toString())?

          Show
          Daryn Sharp added a comment - Glanced over it and the toString() made me ill... Why not: URLDecoder.decode(uri.toString()) ?
          Hide
          Kihwal Lee added a comment -

          org.apache.hadoop.security.token.Token.getKind() is unsynchronized,
          org.apache.hadoop.security.token.Token.setKind(Text) is synchronized

          This findbugs warning is not related to this patch.

          Show
          Kihwal Lee added a comment - org.apache.hadoop.security.token.Token.getKind() is unsynchronized, org.apache.hadoop.security.token.Token.setKind(Text) is synchronized This findbugs warning is not related to this patch.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed unit tests in .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/332//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/332//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/332//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501136/HADOOP-7360.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/332//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/332//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/332//console This message is automatically generated.
          Hide
          Kihwal Lee added a comment -
          1. lookupStat is back to accepting String as an arg. ctors are also back to the way it was in the last patch.
          2. checkIfDirectory: I think now I have made it super clear what is asked.
          3. stringToUri: Changed the scheme parsing. Now the method accepts ":foo/bar" as a scheme-less, authority-less path compoenent. It should not matter since this is not the place to enforce scheme-specific requirements on the following components.
          4. Calls to URI.create() in relativize: Changed to use the safe method.

          Added a safer uriToString method.

          Show
          Kihwal Lee added a comment - lookupStat is back to accepting String as an arg. ctors are also back to the way it was in the last patch. checkIfDirectory : I think now I have made it super clear what is asked. stringToUri : Changed the scheme parsing. Now the method accepts ":foo/bar" as a scheme-less, authority-less path compoenent. It should not matter since this is not the place to enforce scheme-specific requirements on the following components. Calls to URI.create() in relativize : Changed to use the safe method. Added a safer uriToString method.
          Kihwal Lee made changes -
          Attachment HADOOP-7360.txt [ 12501136 ]
          Hide
          Kihwal Lee added a comment -

          I also see a few places that needs improvement. I will incorporate your comment and update the patch soon.

          Show
          Kihwal Lee added a comment - I also see a few places that needs improvement. I will incorporate your comment and update the patch soon.
          Hide
          Daryn Sharp added a comment -

          Compared to earlier patches:

          1. lookupStat was changed to throw a PathNotFoundException with a Path string instead of the string used to instantiate this PathData. Since the two may be different, it defeats part of the reason for this jira.
          2. Minor issue, but changing the method name of checkIfDirectory(...) to checkIfExists(...) reduces the ability, in the context of a caller, to understand what the method does. Perhaps checkIfExistsAndIsDirectory?
          3. In stringToUri(...):
            • Parsing a scheme out when slash == -1 is allowing opaque uris, which hadoop doesn't support, and causes a path that contains a colon to fail.
            • Parsing a scheme should use the condition colon == slash - 1, not colon < slash to avoid attempting parse opaque uris.
          4. Don't the calls to URI.create(...) in relativize(...) also need to be replaced with stringToUri(...)?

          I need to look at it a bit more.

          Show
          Daryn Sharp added a comment - Compared to earlier patches: lookupStat was changed to throw a PathNotFoundException with a Path string instead of the string used to instantiate this PathData . Since the two may be different, it defeats part of the reason for this jira. Minor issue, but changing the method name of checkIfDirectory(...) to checkIfExists(...) reduces the ability, in the context of a caller, to understand what the method does. Perhaps checkIfExistsAndIsDirectory ? In stringToUri(...) : Parsing a scheme out when slash == -1 is allowing opaque uris, which hadoop doesn't support, and causes a path that contains a colon to fail. Parsing a scheme should use the condition colon == slash - 1 , not colon < slash to avoid attempting parse opaque uris. Don't the calls to URI.create(...) in relativize(...) also need to be replaced with stringToUri(...) ? I need to look at it a bit more.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed unit tests in .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/330//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/330//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/330//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501072/HADOOP-7360.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/330//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/330//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/330//console This message is automatically generated.
          Kihwal Lee made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Kihwal Lee added a comment -

          HDFS-2038 needs to be redone to avoid the test failure in HDFS.

          Show
          Kihwal Lee added a comment - HDFS-2038 needs to be redone to avoid the test failure in HDFS.
          Hide
          Kihwal Lee added a comment -

          1. CopyCommands#Get: class instance variable LocalFileSystem localFs is no longer...

          Deleted.

          2. PathData.toFile()

          A check has been added to make sure it is LocalFileSystem. Otherwise an IllegalArgumentException is thrown.

          3. PathData.toString()

          Comment corrected. Actually toString() is okay. The problem was creating the URI. Added a static method to parse and extract each uri component from input string. It is slightly different from what is done in Path.

          4. In PathData ctors, the use of URI.create() may throw undeclared RuntimeExceptions.

          The ctor now throws IOException.

          5. In main ctor, I'm very happy that this.path is initialized to a fully qualified path. Does this work correctly for files that don't exist yet?

          I am too. And Yes.

          6. lookupStat() should be changed to take path rather than pathString as its argument.

          Changed as suggested. this.path is initialized before calling lookupStat() in ctors.

          7. checkIfDirectory(boolean flag): Please change name of argument to "desiredResult" or ...

          now using less confusing names.

          8. getStringForChildPath(): if caller provides an absolute path, this won't behave right. ...

          Relative or absolute, Path#getName() always returns the last component, so it won't matter.

          9. PathType: It's actually "scheme" not "schema". And strictly speaking ...

          Corrected as suggested.

          10. expandAsGlob(): please use "cwd" ...

          Corrected as suggested.

          11. removeAuthority(): I don't understand the comment at the beginning of the method.

          Removed. The comment applies to a different method in URI. A stale one, I guess.

          Show
          Kihwal Lee added a comment - 1. CopyCommands#Get: class instance variable LocalFileSystem localFs is no longer... Deleted. 2. PathData.toFile() A check has been added to make sure it is LocalFileSystem. Otherwise an IllegalArgumentException is thrown. 3. PathData.toString() Comment corrected. Actually toString() is okay. The problem was creating the URI. Added a static method to parse and extract each uri component from input string. It is slightly different from what is done in Path. 4. In PathData ctors, the use of URI.create() may throw undeclared RuntimeExceptions. The ctor now throws IOException. 5. In main ctor, I'm very happy that this.path is initialized to a fully qualified path. Does this work correctly for files that don't exist yet? I am too. And Yes. 6. lookupStat() should be changed to take path rather than pathString as its argument. Changed as suggested. this.path is initialized before calling lookupStat() in ctors. 7. checkIfDirectory(boolean flag): Please change name of argument to "desiredResult" or ... now using less confusing names. 8. getStringForChildPath(): if caller provides an absolute path, this won't behave right. ... Relative or absolute, Path#getName() always returns the last component, so it won't matter. 9. PathType: It's actually "scheme" not "schema". And strictly speaking ... Corrected as suggested. 10. expandAsGlob(): please use "cwd" ... Corrected as suggested. 11. removeAuthority(): I don't understand the comment at the beginning of the method. Removed. The comment applies to a different method in URI. A stale one, I guess.
          Kihwal Lee made changes -
          Attachment HADOOP-7360.txt [ 12501072 ]
          Kihwal Lee made changes -
          Attachment MAPREDUCE-7360.txt [ 12501071 ]
          Kihwal Lee made changes -
          Attachment MAPREDUCE-7360.txt [ 12501071 ]
          Kihwal Lee made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Assignee Daryn Sharp [ daryn ] Kihwal Lee [ kihwal ]
          Daryn Sharp made changes -
          Fix Version/s 0.23.0 [ 12315569 ]
          Hide
          Matt Foley added a comment -

          I think we're very close, but I do have some more questions:

          1. CopyCommands#Get: class instance variable LocalFileSystem localFs is no longer used and should be deleted.
          Also, Get.processPath(PathData,PathData) now assumes that PathData.toFile() returns a file in the LocalFS, so see next question below.

          PathData:

          2. PathData.toFile(): Is it the intent that, if caller passes in a non-local file, the cast to (LocalFileSystem) will throw undeclared ClassCastException? That okay, if so, but let's document it:
          Since the intent (from CopyCommands#Get.processPath() and CopyCommands#Get.copyFileToLocal()) is to create a file in the LocalFS only, and assuming that the cast to (LocalFileSystem) is intended to throw an exception if applied to a non-local fs, please change the method name to "toLocalFile()" and JavaDoc the requirement that this PathData must be for a local path, and "@throws ClassCastException if this.fs is not the LocalFileSystem."

          3. PathData.toString(): the comment says should return the "string" if available, but it just returns uri.toString(), which presumably won't be the same. Specifically, in related method Path.toString() it says:

           
              // we can't use uri.toString(), which escapes everything, because we want 
              // illegal characters unescaped in the string, for glob processing, etc. 
          

          Is this really okay for PathData.toString(), given that your usage specifically requires un-munged globbish characters?

          4. In PathData ctors, the use of URI.create() may throw undeclared RuntimeExceptions. Will this be handled okay by higher-level code?

          5. In main ctor, I'm very happy that this.path is initialized to a fully qualified path. Does this work correctly for files that don't exist yet? (It looks like it probably will, but please confirm you've tested this case.)

          6. lookupStat() should be changed to take path rather than pathString as its argument. This uses the qualified path, which we know was correctly resolved at PathData initialization time. Going thru new Path(uri.toString()) risks re-resolution of relative pathstrings during refreshStatus(), and we can't know if the CWD has changed in the meantime.
          Or do you expect to do lookupStat() on globstrings with singular resolutions? Maybe that case should be dealt with specially if needed.

          7. checkIfDirectory(boolean flag): Please change name of argument to "desiredResult" or "testPolarity" or some such, and JavaDoc @param desiredResult - if true, throws an exception when "this" is not a directory; if false, throws an exception when "this" IS a directory and @throws PathIOException if path doesn't exist or if result of check is not desiredResult

          8. getStringForChildPath(): if caller provides an absolute path, this won't behave right. It should check for the path being a relative path, no? Or could use relativize() if that's the right thing to do, but should probably still throw an exception if it isn't actually a child of this.

          9. PathType: It's actually "scheme" not "schema". And strictly speaking all scheme-having paths are absolute, so ABSOLUTE could be SCHEMELESS_ABSOLUTE (ie, slash-relative) to make the three terms mutually exclusive – but I don't insist on it.

          10. expandAsGlob(): please use "cwd" (current working directory) instead of "pwd" (print working directory).

          11. removeAuthority(): I don't understand the comment at the beginning of the method.

          TestPathData:

          Looks okay.

          Show
          Matt Foley added a comment - I think we're very close, but I do have some more questions: 1. CopyCommands#Get: class instance variable LocalFileSystem localFs is no longer used and should be deleted. Also, Get.processPath(PathData,PathData) now assumes that PathData.toFile() returns a file in the LocalFS, so see next question below. PathData: 2. PathData.toFile(): Is it the intent that, if caller passes in a non-local file, the cast to (LocalFileSystem) will throw undeclared ClassCastException? That okay, if so, but let's document it: Since the intent (from CopyCommands#Get.processPath() and CopyCommands#Get.copyFileToLocal()) is to create a file in the LocalFS only, and assuming that the cast to (LocalFileSystem) is intended to throw an exception if applied to a non-local fs, please change the method name to "toLocalFile()" and JavaDoc the requirement that this PathData must be for a local path, and "@throws ClassCastException if this.fs is not the LocalFileSystem." 3. PathData.toString(): the comment says should return the "string" if available, but it just returns uri.toString(), which presumably won't be the same. Specifically, in related method Path.toString() it says: // we can't use uri.toString(), which escapes everything, because we want // illegal characters unescaped in the string, for glob processing, etc. Is this really okay for PathData.toString(), given that your usage specifically requires un-munged globbish characters? 4. In PathData ctors, the use of URI.create() may throw undeclared RuntimeExceptions. Will this be handled okay by higher-level code? 5. In main ctor, I'm very happy that this.path is initialized to a fully qualified path. Does this work correctly for files that don't exist yet? (It looks like it probably will, but please confirm you've tested this case.) 6. lookupStat() should be changed to take path rather than pathString as its argument. This uses the qualified path, which we know was correctly resolved at PathData initialization time. Going thru new Path(uri.toString()) risks re-resolution of relative pathstrings during refreshStatus(), and we can't know if the CWD has changed in the meantime. Or do you expect to do lookupStat() on globstrings with singular resolutions? Maybe that case should be dealt with specially if needed. 7. checkIfDirectory(boolean flag): Please change name of argument to "desiredResult" or "testPolarity" or some such, and JavaDoc @param desiredResult - if true, throws an exception when "this" is not a directory; if false, throws an exception when "this" IS a directory and @throws PathIOException if path doesn't exist or if result of check is not desiredResult 8. getStringForChildPath(): if caller provides an absolute path, this won't behave right. It should check for the path being a relative path, no? Or could use relativize() if that's the right thing to do, but should probably still throw an exception if it isn't actually a child of this. 9. PathType: It's actually "scheme" not "schema". And strictly speaking all scheme-having paths are absolute, so ABSOLUTE could be SCHEMELESS_ABSOLUTE (ie, slash-relative) to make the three terms mutually exclusive – but I don't insist on it. 10. expandAsGlob(): please use "cwd" (current working directory) instead of "pwd" (print working directory). 11. removeAuthority(): I don't understand the comment at the beginning of the method. TestPathData: Looks okay.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/719//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/719//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/719//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12486201/HADOOP-7360-4.patch against trunk revision 1145525. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/719//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/719//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/719//console This message is automatically generated.
          Daryn Sharp made changes -
          Attachment HADOOP-7360-4.patch [ 12486201 ]
          Hide
          Daryn Sharp added a comment -

          Sorry, stringified the sorted array wrong. I don't have the sorting issue so the test works fine on my machine, hence I didn't notice the error.

          Again, the findbugs warning is in unrelated metrics code.

          Show
          Daryn Sharp added a comment - Sorry, stringified the sorted array wrong. I don't have the sorting issue so the test works fine on my machine, hence I didn't notice the error. Again, the findbugs warning is in unrelated metrics code.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 core unit tests:
          org.apache.hadoop.fs.shell.TestPathData

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/718//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/718//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/718//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12486195/HADOOP-7360-3.patch against trunk revision 1145525. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 core unit tests: org.apache.hadoop.fs.shell.TestPathData +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/718//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/718//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/718//console This message is automatically generated.
          Daryn Sharp made changes -
          Attachment HADOOP-7360-3.patch [ 12486195 ]
          Hide
          Daryn Sharp added a comment -

          Fixes the test failure by sorting the paths prior to comparison to account for unexpected differences in ordering on varying machines.

          Show
          Daryn Sharp added a comment - Fixes the test failure by sorting the paths prior to comparison to account for unexpected differences in ordering on varying machines.
          Hide
          Daryn Sharp added a comment -

          Looking into test case failure, but having trouble reproducing it...

          Show
          Daryn Sharp added a comment - Looking into test case failure, but having trouble reproducing it...
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 core unit tests:
          org.apache.hadoop.fs.shell.TestPathData

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/716//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/716//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/716//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12486098/HADOOP-7360-2.patch against trunk revision 1144858. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 appears to introduce 1 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 core unit tests: org.apache.hadoop.fs.shell.TestPathData +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/716//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/716//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/716//console This message is automatically generated.
          Daryn Sharp made changes -
          Attachment HADOOP-7360-2.patch [ 12486098 ]
          Hide
          Daryn Sharp added a comment -

          Update the tests as suggestion, and added more tests.
          Removed some of the debated methods that are not directly used in this patch.
          Add a few sanity checks to methods that expect an existing directory/not-directory.

          Note: test-patch has flagged a findbugs warning that does not originate in this patch.

          Show
          Daryn Sharp added a comment - Update the tests as suggestion, and added more tests. Removed some of the debated methods that are not directly used in this patch. Add a few sanity checks to methods that expect an existing directory/not-directory. Note: test-patch has flagged a findbugs warning that does not originate in this patch.
          Hide
          Matt Foley added a comment -

          It would be a very bad idea to remove the method PathData(FileSystem, String, FileStatus) ... avoid unnecessary re-stat of the path ...

          Understood. As long as it's private, that's fine. Please comment it as being for that use.

          Ok to the rest of the responses. Comments on how to make relativity safe are in HDFS-7393. Thanks, Daryn.

          Show
          Matt Foley added a comment - It would be a very bad idea to remove the method PathData(FileSystem, String, FileStatus) ... avoid unnecessary re-stat of the path ... Understood. As long as it's private, that's fine. Please comment it as being for that use. Ok to the rest of the responses. Comments on how to make relativity safe are in HDFS-7393. Thanks, Daryn.
          Hide
          Daryn Sharp added a comment -

          I can address the test points. One question: If I need to construct a path of testdir+"/d1/f1", is this the preferred way: new Path(new Path(testDir, "d1"), "f1")?

          Why did you remove testWithFsAndPath()

          It's private now.

          [...] suggest also eliminating PathData(FileSystem, String, FileStatus) and PathData(FileSystem, FileStatus). Basically, the status should be looked up in the FS, not set as an argument.

          PathData(FileSystem, FileStatus) was removed in the patch. Did you mean something else?

          It would be a very bad idea to remove the method PathData(FileSystem, String, FileStatus). It's private, and only used in cases like listStatus/globStatus where the result is an array of FileStatus. It's used to avoid an unnecessary re-stat of the path – overall, this class has drastically reduced the unnecessary re-stats occurring in FsShell.

          PathData(FileSystem, String) [...] becomes the primary ctor.

          PathData(FileSystem,String,FileStatus) is the primary ctor because a) removing it will cause 2X the stats, b) it extracts path from a non-null status. The path is a final, so I can't invoke super(FileSystem, String) and then reset the path.

          setStat(boolean ignoreFNF) is counter-intuitive. [...] Please call it lookupStatus(boolean ignoreFNF) [...]

          100% agreed! I'm actually surprised I did that... Either I was lacking coffee, or slipped up an eclipse refactor...

          getChecksumFile(): Why is this method needed (returns PathData), vs just using Fs.getChecksumFile() (returns Path)?

          There's another change backed up behind this one that drastically simplifies the copy commands to operate purely on PathData (which incidentally will further reduce stats, but that's not the primary purpose). Since PathData(FileSystem,String) is now private, I had to push it into PathData, else I have to re-expose the private ctor which will complicate the switch to FileContext. Since commands aren't storing the fs in temporaries, it may be possible to just switch fs to a FileContext and generally have everything work. Having a command monkeying around getting a raw fs for the crc file and then instantiating thru what what should be a private ctor is a minor setback.

          Why is this method needed instead of just using Path.getParent()? There are currently no callers of this new method.

          There are backed up changes that depend on it (I didn't expect this jira to languish...). Remember that everything is operating on PathData, so I can't use Path.getParent() because I'd have to re-expose a private ctor.

          The other questions doubting the need for relativity are addressed on the jira you filed. My design goal has been to mimic a unix shell.

          This just begs for such mis-use

          I think computer users are well enough acquainted with filesystems to know that if you change the pwd, then relative paths are no longer valid.

          Show
          Daryn Sharp added a comment - I can address the test points. One question: If I need to construct a path of testdir+"/d1/f1" , is this the preferred way: new Path(new Path(testDir, "d1"), "f1") ? Why did you remove testWithFsAndPath() It's private now. [...] suggest also eliminating PathData(FileSystem, String, FileStatus) and PathData(FileSystem, FileStatus). Basically, the status should be looked up in the FS, not set as an argument. PathData(FileSystem, FileStatus) was removed in the patch. Did you mean something else? It would be a very bad idea to remove the method PathData(FileSystem, String, FileStatus) . It's private, and only used in cases like listStatus/globStatus where the result is an array of FileStatus . It's used to avoid an unnecessary re-stat of the path – overall, this class has drastically reduced the unnecessary re-stats occurring in FsShell. PathData(FileSystem, String) [...] becomes the primary ctor. PathData(FileSystem,String,FileStatus) is the primary ctor because a) removing it will cause 2X the stats, b) it extracts path from a non-null status. The path is a final, so I can't invoke super(FileSystem, String) and then reset the path. setStat(boolean ignoreFNF) is counter-intuitive. [...] Please call it lookupStatus(boolean ignoreFNF) [...] 100% agreed! I'm actually surprised I did that... Either I was lacking coffee, or slipped up an eclipse refactor... getChecksumFile(): Why is this method needed (returns PathData), vs just using Fs.getChecksumFile() (returns Path)? There's another change backed up behind this one that drastically simplifies the copy commands to operate purely on PathData (which incidentally will further reduce stats, but that's not the primary purpose). Since PathData(FileSystem,String) is now private, I had to push it into PathData, else I have to re-expose the private ctor which will complicate the switch to FileContext . Since commands aren't storing the fs in temporaries, it may be possible to just switch fs to a FileContext and generally have everything work. Having a command monkeying around getting a raw fs for the crc file and then instantiating thru what what should be a private ctor is a minor setback. Why is this method needed instead of just using Path.getParent()? There are currently no callers of this new method. There are backed up changes that depend on it (I didn't expect this jira to languish...). Remember that everything is operating on PathData, so I can't use Path.getParent() because I'd have to re-expose a private ctor. The other questions doubting the need for relativity are addressed on the jira you filed. My design goal has been to mimic a unix shell. This just begs for such mis-use I think computer users are well enough acquainted with filesystems to know that if you change the pwd, then relative paths are no longer valid.
          Hide
          Matt Foley added a comment -

          TestPathData:

          Multiple places: The preferred way to express "new Path(testDir+"/d1")" is "new Path(testDir, "d1")". This avoids hard-coding the path delimiter for a local filesystem path.

          initialize():

          • Consider "fs.createNewFile(Path)" instead of "fs.create(Path).close()"
          • If you put the "setWorkingDirectory" command near the beginning of the method, wouldn't all the creates and mkdirs get simpler?

          I'm concerned that testDir is a relative path. Can you convert it to an absolute path as early in the process as possible, before anyone might have set a non-default working directory?

          Why did you remove testWithFsAndPath()? It is not a null test.

          getParent() should be named testGetParent().

          testRelativeGlobBack(): see previous concern about making "testDir" absolute before interacting with second and later setWorkingDirectory() invocations. This might give you testDir/testDir/d1 ?

          PathData:

          Agree with your elimination of PathData(FileSystem, Path, FileStatus),
          but suggest also eliminating PathData(FileSystem, String, FileStatus)
          and PathData(FileSystem, FileStatus).
          Basically, the status should be looked up in the FS, not set as an argument.
          Should do so at the end of method PathData(FileSystem, String).
          In comment for PathData(FileSystem, String), remove "Convenience" adjective. It becomes the primary ctor.

          setStat(boolean ignoreFNF) is counter-intuitive. Most people seeing "setStat(boolean)" will assume the status is being set to the argument. Please call it lookupStatus(boolean ignoreFNF). Or could use two methods, both with zero arguments, called setStat() and setStatIgnoreFNF().

          Suggest letting lookupStatus(ignoreFNF) return the stat value instead of void. Then refreshStatus just becomes "return lookupStatus(false);".

          getChecksumFile(): Why is this method needed (returns PathData), vs just using Fs.getChecksumFile() (returns Path)?

          getParent(): Several concerns:

          • Why is this method needed instead of just using Path.getParent()? There are currently no callers of this new method.
          • Why is it important to preserve the relative-ness of the parent path? Trying to do so exposes you to serious issues because the current working directory may have changed since PathData#string was stored, which means the value returned by this method could be meaningless if string was relative. I think I would go so far as to say this can't work. I would much rather let Path.getParent() do what it was carefully implemented to do.
          • The same issue applies to the next two chunks.

          This brings up the issue of why PathData saves the value of #string?
          This just begs for such mis-use. I've opened HADOOP-7393 for this.

          Show
          Matt Foley added a comment - TestPathData: Multiple places: The preferred way to express "new Path(testDir+"/d1")" is "new Path(testDir, "d1")". This avoids hard-coding the path delimiter for a local filesystem path. initialize(): Consider "fs.createNewFile(Path)" instead of "fs.create(Path).close()" If you put the "setWorkingDirectory" command near the beginning of the method, wouldn't all the creates and mkdirs get simpler? I'm concerned that testDir is a relative path. Can you convert it to an absolute path as early in the process as possible, before anyone might have set a non-default working directory? Why did you remove testWithFsAndPath()? It is not a null test. getParent() should be named testGetParent(). testRelativeGlobBack(): see previous concern about making "testDir" absolute before interacting with second and later setWorkingDirectory() invocations. This might give you testDir/testDir/d1 ? PathData: Agree with your elimination of PathData(FileSystem, Path, FileStatus), but suggest also eliminating PathData(FileSystem, String, FileStatus) and PathData(FileSystem, FileStatus). Basically, the status should be looked up in the FS, not set as an argument. Should do so at the end of method PathData(FileSystem, String). In comment for PathData(FileSystem, String), remove "Convenience" adjective. It becomes the primary ctor. setStat(boolean ignoreFNF) is counter-intuitive. Most people seeing "setStat(boolean)" will assume the status is being set to the argument. Please call it lookupStatus(boolean ignoreFNF). Or could use two methods, both with zero arguments, called setStat() and setStatIgnoreFNF(). Suggest letting lookupStatus(ignoreFNF) return the stat value instead of void. Then refreshStatus just becomes "return lookupStatus(false);". getChecksumFile(): Why is this method needed (returns PathData), vs just using Fs.getChecksumFile() (returns Path)? getParent(): Several concerns: Why is this method needed instead of just using Path.getParent()? There are currently no callers of this new method. Why is it important to preserve the relative-ness of the parent path? Trying to do so exposes you to serious issues because the current working directory may have changed since PathData#string was stored, which means the value returned by this method could be meaningless if string was relative. I think I would go so far as to say this can't work. I would much rather let Path.getParent() do what it was carefully implemented to do. The same issue applies to the next two chunks. This brings up the issue of why PathData saves the value of #string? This just begs for such mis-use. I've opened HADOOP-7393 for this.
          Daryn Sharp made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Daryn Sharp made changes -
          Attachment HADOOP-7360.patch [ 12481540 ]
          Hide
          Daryn Sharp added a comment -

          PathData retains all forms of relativity in a glob, including whether the authority is present or absent (ie. hdfs://host/path or hdfs:///path).

          Streamlines the PathData ctors, and converts most to private since they were only public to ease the redesign.

          Changed CLITestHelper to expand keywords not only in the commands, but also in the expected output. Notably this allows NAMENODE to be used in the expected output instead of fuzzy regexps.

          Show
          Daryn Sharp added a comment - PathData retains all forms of relativity in a glob, including whether the authority is present or absent (ie. hdfs://host/path or hdfs:///path). Streamlines the PathData ctors, and converts most to private since they were only public to ease the redesign. Changed CLITestHelper to expand keywords not only in the commands, but also in the expected output. Notably this allows NAMENODE to be used in the expected output instead of fuzzy regexps.
          Daryn Sharp made changes -
          Link This issue blocks HDFS-2038 [ HDFS-2038 ]
          Daryn Sharp made changes -
          Field Original Value New Value
          Link This issue is part of HADOOP-7176 [ HADOOP-7176 ]
          Daryn Sharp created issue -

            People

            • Assignee:
              Kihwal Lee
              Reporter:
              Daryn Sharp
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development