Hadoop Common
  1. Hadoop Common
  2. HADOOP-7320

Refactor FsShell's copy & move commands

    Details

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

      Description

      Need to refactor the move and copy commands to conform to the FsCommand class.

      1. HADOOP-7320.patch
        53 kB
        Daryn Sharp
      2. HADOOP-7320-2.patch
        55 kB
        Daryn Sharp
      3. HADOOP-7320-3.patch
        58 kB
        Daryn Sharp
      4. HADOOP-7320-rename.patch
        3 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #700 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/700/)
          HADOOP-7320. Refactor the copy and move commands to conform to new FsCommand class. Contributed by Daryn Sharp.

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

          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java
          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandWithDestination.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/MoveCommands.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathExceptions.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Copy.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFsShellReturnCode.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #700 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/700/ ) HADOOP-7320 . Refactor the copy and move commands to conform to new FsCommand class. Contributed by Daryn Sharp. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127591 Files : /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandWithDestination.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/MoveCommands.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathExceptions.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Copy.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFsShellReturnCode.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #619 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/619/)
          HADOOP-7320. Refactor the copy and move commands to conform to new FsCommand class. Contributed by Daryn Sharp.

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

          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java
          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathData.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandWithDestination.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/MoveCommands.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathExceptions.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Ls.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Copy.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFsShellReturnCode.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #619 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/619/ ) HADOOP-7320 . Refactor the copy and move commands to conform to new FsCommand class. Contributed by Daryn Sharp. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127591 Files : /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathData.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandWithDestination.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/MoveCommands.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/PathExceptions.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Ls.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Copy.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFsShellReturnCode.java
          Hide
          Todd Lipcon added a comment -

          Committed to trunk. Thanks, Daryn!

          Show
          Todd Lipcon added a comment - Committed to trunk. Thanks, Daryn!
          Hide
          Todd Lipcon added a comment -

          +1. Hudson doesn't know how to deal with the renamed file in the test-patch process, but I re-ran tests locally.

          Show
          Todd Lipcon added a comment - +1. Hudson doesn't know how to deal with the renamed file in the test-patch process, but I re-ran tests locally.
          Hide
          Hadoop QA added a comment -

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

          +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 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +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:

          -1 system test framework. The patch failed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/515//testReport/
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/515//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/12480293/HADOOP-7320-rename.patch against trunk revision 1126719. +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 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +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: -1 system test framework. The patch failed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/515//testReport/ Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/515//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          This patch must be applied first to allow the real patch to rename Copy.java to CopyCommands.java.

          Show
          Daryn Sharp added a comment - This patch must be applied first to allow the real patch to rename Copy.java to CopyCommands.java.
          Hide
          Daryn Sharp added a comment -

          The patch problem is due to renaming Copy to CopyCommands per review comments. Subversion is generating a patch that it can't reapply. Namely that the patch deletes Copy, but the hunks for CopyCommands are relative to the Copy command.

          Should I regenerate the patch by svn removing and adding the renamed file? Since that will lose history (it's a very new file so maybe it doesn't matter), is there another way to generate the patch?

          Show
          Daryn Sharp added a comment - The patch problem is due to renaming Copy to CopyCommands per review comments. Subversion is generating a patch that it can't reapply. Namely that the patch deletes Copy, but the hunks for CopyCommands are relative to the Copy command. Should I regenerate the patch by svn removing and adding the renamed file? Since that will lose history (it's a very new file so maybe it doesn't matter), is there another way to generate the 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/12480183/HADOOP-7320-3.patch
          against trunk revision 1126719.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/510//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/12480183/HADOOP-7320-3.patch against trunk revision 1126719. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/510//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -
          • Fix "pathson" misspelling.
          • Rename Copy to CopyCommands, ditto for Move.
          • Removed "extends" from Copy/MoveCommands. It was a leftover from when I had common methods in the class, prior to moving them to CommandWithDestination.
          Show
          Daryn Sharp added a comment - Fix "pathson" misspelling. Rename Copy to CopyCommands, ditto for Move. Removed "extends" from Copy/MoveCommands. It was a leftover from when I had common methods in the class, prior to moving them to CommandWithDestination.
          Hide
          Daryn Sharp added a comment -

          Can you explain the changes with regard to RawLocalFileSystem vs LocalFileSystem in TestFsShellReturnCode? I'm not sure I follow what's going on here.

          It was confusing for me too. The property fs.file.impl is supposed to be a LocalFileSystem, but the test was defining it as a RawLocalFileSystem. Calls to FileSystem.getLocal() instantiate the fs.file.impl class and then cast it to a LocalFileSystem (runtime kaboom). Using the wrong class accidentally worked until this patch.

          I'll address the last 2 bullet points on this jira.

          Show
          Daryn Sharp added a comment - Can you explain the changes with regard to RawLocalFileSystem vs LocalFileSystem in TestFsShellReturnCode? I'm not sure I follow what's going on here. It was confusing for me too. The property fs.file.impl is supposed to be a LocalFileSystem, but the test was defining it as a RawLocalFileSystem. Calls to FileSystem.getLocal() instantiate the fs.file.impl class and then cast it to a LocalFileSystem (runtime kaboom). Using the wrong class accidentally worked until this patch. I'll address the last 2 bullet points on this jira.
          Hide
          Todd Lipcon added a comment -
          • Can you explain the changes with regard to RawLocalFileSystem vs LocalFileSystem in TestFsShellReturnCode? I'm not sure I follow what's going on here.
          • typo "pathson" -> "paths on" for Rename
          • maybe rename the outer "Copy" class to "CopyCommands" since it's just a container? Also, should it really be extending FsCommand since it is not itself a command? (I realize these are left over from HADOOP-7251... can address in a separate JIRA if you prefer)
          Show
          Todd Lipcon added a comment - Can you explain the changes with regard to RawLocalFileSystem vs LocalFileSystem in TestFsShellReturnCode? I'm not sure I follow what's going on here. typo "pathson" -> "paths on" for Rename maybe rename the outer "Copy" class to "CopyCommands" since it's just a container? Also, should it really be extending FsCommand since it is not itself a command? (I realize these are left over from HADOOP-7251 ... can address in a separate JIRA if you prefer)
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12480166/HADOOP-7320-2.patch
          against trunk revision 1126719.

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

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

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

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

          +1 findbugs. The patch does not introduce any new Findbugs (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/hudson/job/PreCommit-HADOOP-Build/508//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/508//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/508//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/12480166/HADOOP-7320-2.patch against trunk revision 1126719. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (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/hudson/job/PreCommit-HADOOP-Build/508//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/508//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/508//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Fix the test that failed

          Show
          Daryn Sharp added a comment - Fix the test that failed
          Hide
          Hadoop QA added a comment -

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

          +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 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 core unit tests:
          org.apache.hadoop.fs.TestFsShellReturnCode

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/507//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/507//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/507//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/12480149/HADOOP-7320.patch against trunk revision 1126287. +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 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 core unit tests: org.apache.hadoop.fs.TestFsShellReturnCode +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/507//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/507//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/507//console This message is automatically generated.

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Daryn Sharp
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development