Hadoop Common
  1. Hadoop Common
  2. HADOOP-6222

Core doesn't have TestCommonCLI facility

    Details

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

      Description

      TestCLI is a base class, which cannot run FS type of commands.
      We need a "copy" of TestHDFSCLI as TestCommonCLI to be able to test CLI stuff in common.

      I suggest we create TestCommonCLI.java in hadoop-common

      1. HADOOP-6222.patch
        4 kB
        Konstantin Boudnik
      2. HADOOP-6222.patch
        4 kB
        Konstantin Boudnik
      3. HADOOP-6222.patch
        7 kB
        Konstantin Boudnik
      4. HADOOP-6222.patch
        7 kB
        Konstantin Boudnik
      5. HADOOP-6222.patch
        7 kB
        Konstantin Boudnik
      6. HADOOP-6222.patch
        37 kB
        Konstantin Boudnik
      7. HADOOP-6222.hdfs-part.patch
        10 kB
        Konstantin Boudnik
      8. HADOOP-6222.hdfs-part.patch
        10 kB
        Konstantin Boudnik
      9. HADOOP-6222.hdfs-part.patch
        9 kB
        Konstantin Boudnik
      10. HADOOP-6222_hdfs_part.patch
        4 kB
        Konstantin Boudnik
      11. HADOOP-6222_hdfs_part.patch
        4 kB
        Konstantin Boudnik

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #94 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/94/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #94 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/94/ )
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #148 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/148/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #148 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/148/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #172 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/172/)
          HDFS-832. HDFS side of . Contributed by Konstantin Boudnik.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #172 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/172/ ) HDFS-832 . HDFS side of . Contributed by Konstantin Boudnik.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #189 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/189/)
          . Core doesn't have TestCommonCLI facility. Contributed by Konstantin Boudnik.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #189 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/189/ ) . Core doesn't have TestCommonCLI facility. Contributed by Konstantin Boudnik.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #148 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/148/)
          HDFS-832. HDFS side of . Contributed by Konstantin Boudnik.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #148 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/148/ ) HDFS-832 . HDFS side of . Contributed by Konstantin Boudnik.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #117 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/117/)
          . Core doesn't have TestCommonCLI facility. Contributed by Konstantin Boudnik.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #117 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/117/ ) . Core doesn't have TestCommonCLI facility. Contributed by Konstantin Boudnik.
          Hide
          Konstantin Boudnik added a comment -

          I've just committed this.
          NB: this commit will cause HDFS builds to fail for a short time. I have a separate patch for HDFS which will be tested and committed shortly after this one is integrated into new Common snapshot.

          Show
          Konstantin Boudnik added a comment - I've just committed this. NB: this commit will cause HDFS builds to fail for a short time. I have a separate patch for HDFS which will be tested and committed shortly after this one is integrated into new Common snapshot.
          Hide
          Boris Shkolnik added a comment -

          +1

          Show
          Boris Shkolnik added a comment - +1
          Hide
          Hadoop QA added a comment -

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

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

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

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/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/12427985/HADOOP-6222.patch against trunk revision 890588. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 23 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/204/console This message is automatically generated.
          Hide
          Konstantin Boudnik added a comment -

          Seems to be Ok to run it through the verification process one more time.

          Show
          Konstantin Boudnik added a comment - Seems to be Ok to run it through the verification process one more time.
          Hide
          Konstantin Boudnik added a comment -

          After an exchange with Boris I've added a trivial test for fs -help command to the patch. Now it has one test case to demonstrate that the refactoring works as expected.

          It should be clear though that only the tests which do not require a cluster to be up and running and can run with a local filesystem can be placed into Common. Otherwise they are going to fail.

          Show
          Konstantin Boudnik added a comment - After an exchange with Boris I've added a trivial test for fs -help command to the patch. Now it has one test case to demonstrate that the refactoring works as expected. It should be clear though that only the tests which do not require a cluster to be up and running and can run with a local filesystem can be placed into Common. Otherwise they are going to fail.
          Hide
          Konstantin Boudnik added a comment -

          I've ran local test-patch verification for Common part and all seems cool:

          There appear to be 1 release audit warnings before the patch and 0 release audit warnings after applying the patch.
          
          -1 overall.
              +1 @author.  The patch does not contain any @author tags.
              +1 tests included.  The patch appears to include 12 new or modified tests.
              +1 javadoc.  The javadoc tool did not generate any warning messages.
              -1 javac.  The patch appears to cause tar ant target to fail.
              +1 findbugs.  The patch does not introduce any new Findbugs warnings.
              +1 release audit.  The applied patch does not increase the total number of release audit warnings.
          

          The warning shown is related to the TestCLI class renaming, which has to be addressed by SVN not patch program. Tests ran just fine.

          There two ways of integrating this:

          • do verification of HDFS part as soon as Common is committed (will cause HDFS builds to fail until its respective part is committed)
          • commit both at once (no problem is expected in this case)

          I'd suggest to go with latter approach.

          Show
          Konstantin Boudnik added a comment - I've ran local test-patch verification for Common part and all seems cool: There appear to be 1 release audit warnings before the patch and 0 release audit warnings after applying the patch. -1 overall. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. The warning shown is related to the TestCLI class renaming, which has to be addressed by SVN not patch program. Tests ran just fine. There two ways of integrating this: do verification of HDFS part as soon as Common is committed (will cause HDFS builds to fail until its respective part is committed) commit both at once (no problem is expected in this case) I'd suggest to go with latter approach.
          Hide
          Konstantin Boudnik added a comment -

          Addressing Boris' comments but one:

          • the problem with a test in current Common is that all existing CLI tests require MiniDFSCluster to be executed. Which is clearly doesn't fit Common testing paradigm.

          I also have ran TestMRCLI with new common-test.jar and hdfs-test.jar and everything passed as is without any modifications on MR side.

          Show
          Konstantin Boudnik added a comment - Addressing Boris' comments but one: the problem with a test in current Common is that all existing CLI tests require MiniDFSCluster to be executed. Which is clearly doesn't fit Common testing paradigm. I also have ran TestMRCLI with new common-test.jar and hdfs-test.jar and everything passed as is without any modifications on MR side.
          Hide
          Boris Shkolnik added a comment -

          HADOOP-6222.patch
          1. patch file names start with ../common/src, and it should start with src
          2. don't we need at least one test in common to test this?

          HADOOP-6222.hdfs-part.patch
          1. Nit. no new line at the end of CMDFactoryDFS.java
          2. in testStorageRestore - assert<False,True> calls should have different messages for different cases. Right now they all say "after check call"....
          3.Nit. TestHDFSCli has unremoved comment in execute() method.

          Show
          Boris Shkolnik added a comment - HADOOP-6222 .patch 1. patch file names start with ../common/src, and it should start with src 2. don't we need at least one test in common to test this? HADOOP-6222 .hdfs-part.patch 1. Nit. no new line at the end of CMDFactoryDFS.java 2. in testStorageRestore - assert<False,True> calls should have different messages for different cases. Right now they all say "after check call".... 3.Nit. TestHDFSCli has unremoved comment in execute() method.
          Hide
          Konstantin Boudnik added a comment -

          Making factory classes abstract.

          Show
          Konstantin Boudnik added a comment - Making factory classes abstract.
          Hide
          Konstantin Boudnik added a comment -

          New version of the patch addresses all concerns of this JIRA's subtasks:

          • TestCLI is renamed to CLITestHelper to avoid it being picked up by JUnit
          • tests are converted to JUnit4 format
          • check for 0-length test suite is added (thanks Todd)
            Also, it fixes TestStorageRestore which has direct dependencies from TestHDFSCLI.

          Because this patch affects both Common and HDFS there isn't easy way to test HDFS part of it through test-patch unless Common's is committed. I've build and successfully tested Common with the patch in place. Then, using fresh hadoop-core-test.jar I've ran TestHDFSCLI and TestStorageRestore on the patched version of HDFS. All tests were executed successfully.

          Also, I have ran test-patch locally for the Common's modification and it was Ok except for one warning caused by the fact that patch doesn't reflect renaming of TestCLI - it something which has to be done trough SVN.

          Show
          Konstantin Boudnik added a comment - New version of the patch addresses all concerns of this JIRA's subtasks: TestCLI is renamed to CLITestHelper to avoid it being picked up by JUnit tests are converted to JUnit4 format check for 0-length test suite is added (thanks Todd) Also, it fixes TestStorageRestore which has direct dependencies from TestHDFSCLI . Because this patch affects both Common and HDFS there isn't easy way to test HDFS part of it through test-patch unless Common's is committed. I've build and successfully tested Common with the patch in place. Then, using fresh hadoop-core-test.jar I've ran TestHDFSCLI and TestStorageRestore on the patched version of HDFS. All tests were executed successfully. Also, I have ran test-patch locally for the Common's modification and it was Ok except for one warning caused by the fact that patch doesn't reflect renaming of TestCLI - it something which has to be done trough SVN.
          Hide
          Konstantin Boudnik added a comment -

          Addressing Boris' comment - makes sense.

          Show
          Konstantin Boudnik added a comment - Addressing Boris' comment - makes sense.
          Hide
          Boris Shkolnik added a comment -

          Do we really need DFSAdminCmdExecutor class in this case? Can't we just use CLICommands.FSCmdExecutor in both cases?

          Show
          Boris Shkolnik added a comment - Do we really need DFSAdminCmdExecutor class in this case? Can't we just use CLICommands.FSCmdExecutor in both cases?
          Hide
          Konstantin Boudnik added a comment -

          Just for the time being I'm posting two patches (for Common and HDFS) to the same JIRA. I'll open new JIRA for HDFS later on.

          Common patch will have to be applied first and the after core-test jar is propagated through the central repository HDFS patch will have to be applied. Otherwise, HDFS build will be broken with missing classes compilation error.

          I've moved FsCmd related code to Common and have generalized instantiation of Fs and Dfs commands wherever possible

          Show
          Konstantin Boudnik added a comment - Just for the time being I'm posting two patches (for Common and HDFS) to the same JIRA. I'll open new JIRA for HDFS later on. Common patch will have to be applied first and the after core-test jar is propagated through the central repository HDFS patch will have to be applied. Otherwise, HDFS build will be broken with missing classes compilation error. I've moved FsCmd related code to Common and have generalized instantiation of Fs and Dfs commands wherever possible
          Hide
          Konstantin Boudnik added a comment -

          Closer examination of the classes hierarchy showed that DFSAdminCmd processing can't be moved to Coomon without moving DFSAdmin as well. Moving that class up to the Common is clearly undesirable.

          Show
          Konstantin Boudnik added a comment - Closer examination of the classes hierarchy showed that DFSAdminCmd processing can't be moved to Coomon without moving DFSAdmin as well. Moving that class up to the Common is clearly undesirable.
          Hide
          Boris Shkolnik added a comment -

          I agree with Konstantin that we shouldn't copy the code.
          But since we need to support both FS and DFSADMIN commands in common, we may need to either move most of the functionality from TestHDFSCLI to TestCLI or create another class (something like TestShellCLI), move FS/DFSADMIN command processing into it and extend it for HDFS.

          Show
          Boris Shkolnik added a comment - I agree with Konstantin that we shouldn't copy the code. But since we need to support both FS and DFSADMIN commands in common, we may need to either move most of the functionality from TestHDFSCLI to TestCLI or create another class (something like TestShellCLI), move FS/DFSADMIN command processing into it and extend it for HDFS.
          Hide
          Konstantin Boudnik added a comment -

          It sounds great, but it shouldn't be a literal 'copy' cause it's a sure way for code duplication and creating a very different version of the little framework in the future.

          In the past, I was able to pull-off TestCLI from HDFS and make it independent for the sake of Owl project. I'm pretty sure something similar has to be repeated in this case.

          Show
          Konstantin Boudnik added a comment - It sounds great, but it shouldn't be a literal 'copy' cause it's a sure way for code duplication and creating a very different version of the little framework in the future. In the past, I was able to pull-off TestCLI from HDFS and make it independent for the sake of Owl project. I'm pretty sure something similar has to be repeated in this case.

            People

            • Assignee:
              Konstantin Boudnik
              Reporter:
              Boris Shkolnik
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development