Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha1
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: balancer & mover
    • Labels:
      None

      Description

      DiskBalancer CLI invokes CLI functions directly instead of shell. This is not representative of how end users use it. To provide good unit test coverage, we need to have tests where DiskBalancer CLI is invoked via shell.

      1. HDFS-10599.002.patch
        5 kB
        Manoj Govindassamy
      2. HDFS-10599.001.patch
        4 kB
        Manoj Govindassamy

        Activity

        Hide
        manojg Manoj Govindassamy added a comment -

        Thanks for the review and commit help Anu Engineer.

        Show
        manojg Manoj Govindassamy added a comment - Thanks for the review and commit help Anu Engineer .
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10434 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10434/)
        HDFS-10599. DiskBalancer: Execute CLI via Shell. Contributed by Manoj (aengineer: rev e3f7f58a5fb3e18fe6e603ce5018eb805f170d09)

        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/diskbalancer/command/TestDiskBalancerCommand.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DiskBalancerCLI.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10434 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10434/ ) HDFS-10599 . DiskBalancer: Execute CLI via Shell. Contributed by Manoj (aengineer: rev e3f7f58a5fb3e18fe6e603ce5018eb805f170d09) (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/diskbalancer/command/TestDiskBalancerCommand.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DiskBalancerCLI.java
        Hide
        anu Anu Engineer added a comment -

        Manoj Govindassamy Thanks for the contribution. I have committed this to trunk.

        Show
        anu Anu Engineer added a comment - Manoj Govindassamy Thanks for the contribution. I have committed this to trunk.
        Hide
        anu Anu Engineer added a comment -

        +1, I will commit this shortly.

        Show
        anu Anu Engineer added a comment - +1, I will commit this shortly.
        Hide
        manojg Manoj Govindassamy added a comment -

        >> org.apache.hadoop.hdfs.TestDFSShell.testMoveWithTargetPortEmpty
        Unit test failure is not related to v002 patch.

        Show
        manojg Manoj Govindassamy added a comment - >> org.apache.hadoop.hdfs.TestDFSShell.testMoveWithTargetPortEmpty Unit test failure is not related to v002 patch.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 12s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 35s trunk passed
        +1 compile 0m 45s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 50s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 1m 39s trunk passed
        +1 javadoc 0m 54s trunk passed
        +1 mvninstall 0m 46s the patch passed
        +1 compile 0m 41s the patch passed
        +1 javac 0m 41s the patch passed
        +1 checkstyle 0m 21s the patch passed
        +1 mvnsite 0m 47s the patch passed
        +1 mvneclipse 0m 9s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 45s the patch passed
        +1 javadoc 0m 53s the patch passed
        -1 unit 58m 16s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        76m 36s



        Reason Tests
        Failed junit tests hadoop.hdfs.TestDFSShell



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-10599
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828155/HDFS-10599.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 10d258c87402 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 72dfb04
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/16727/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16727/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16727/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 35s trunk passed +1 compile 0m 45s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 50s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 39s trunk passed +1 javadoc 0m 54s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 41s the patch passed +1 javac 0m 41s the patch passed +1 checkstyle 0m 21s the patch passed +1 mvnsite 0m 47s the patch passed +1 mvneclipse 0m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 45s the patch passed +1 javadoc 0m 53s the patch passed -1 unit 58m 16s hadoop-hdfs in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 76m 36s Reason Tests Failed junit tests hadoop.hdfs.TestDFSShell Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10599 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828155/HDFS-10599.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 10d258c87402 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 72dfb04 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16727/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16727/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16727/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        manojg Manoj Govindassamy added a comment -

        Thanks for the review Xiaobing Zhou.

        1. Yes, Report command doesn't need 'fs' option. But 'fs' being a generic option, all Commands should work well with 'fs' option. Since DiskBalancer methods were run directly from the TestBalancerCommand unit tests, generic options weren't added during the run and the Commands used to fail with unexpected option errors. So, the intention of this jira is to make the test run DiskBalancer Commands just like the way it runs in real command line shell via DiskBalancerCLI main and there by have access to all generic options. So, added a unit test to prove DiskBalancer Commands can be run with GenericOptions like "fs".

        2. Thats right. Removed the out parameter from DiskBalancerCLI#dispatch. Thanks for catching this.

        Attaching v002 patch with more test comments and with redundant out argument removed.

        Show
        manojg Manoj Govindassamy added a comment - Thanks for the review Xiaobing Zhou . 1. Yes, Report command doesn't need 'fs' option. But 'fs' being a generic option, all Commands should work well with 'fs' option. Since DiskBalancer methods were run directly from the TestBalancerCommand unit tests, generic options weren't added during the run and the Commands used to fail with unexpected option errors. So, the intention of this jira is to make the test run DiskBalancer Commands just like the way it runs in real command line shell via DiskBalancerCLI main and there by have access to all generic options. So, added a unit test to prove DiskBalancer Commands can be run with GenericOptions like "fs". 2. Thats right. Removed the out parameter from DiskBalancerCLI#dispatch. Thanks for catching this. Attaching v002 patch with more test comments and with redundant out argument removed.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        Thanks Manoj Govindassamy for the work, and Anu Engineer for the review. The patch 001 looks good. Minor comments:
        1. report command doesn't need fs option

        146	    final String topReportArg = "5";
        147	    final String reportArgs = String.format("%s %s -%s -%s %s",
        148	        "fs", cluster.getNameNode().getNameNodeAddressHostPortString(),
        

        2. Since PrintStream is instance member in DiskBalancerCLI, the parameter 'out' can be removed from DiskBalancerCLI#dispatch

        Show
        xiaobingo Xiaobing Zhou added a comment - Thanks Manoj Govindassamy for the work, and Anu Engineer for the review. The patch 001 looks good. Minor comments: 1. report command doesn't need fs option 146 final String topReportArg = "5" ; 147 final String reportArgs = String .format( "%s %s -%s -%s %s" , 148 "fs" , cluster.getNameNode().getNameNodeAddressHostPortString(), 2. Since PrintStream is instance member in DiskBalancerCLI, the parameter 'out' can be removed from DiskBalancerCLI#dispatch
        Hide
        manojg Manoj Govindassamy added a comment -

        Thanks for the review Anu Engineer.

        Show
        manojg Manoj Govindassamy added a comment - Thanks for the review Anu Engineer .
        Hide
        anu Anu Engineer added a comment -

        +1, LGTM. I will commit this tomorrow to make sure that there are no other comments on this JIRA. Manoj Govindassamy Thank you for the contribution.

        Show
        anu Anu Engineer added a comment - +1, LGTM. I will commit this tomorrow to make sure that there are no other comments on this JIRA. Manoj Govindassamy Thank you for the contribution.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 14s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 7m 12s trunk passed
        +1 compile 0m 49s trunk passed
        +1 checkstyle 0m 27s trunk passed
        +1 mvnsite 0m 55s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 46s trunk passed
        +1 javadoc 1m 2s trunk passed
        +1 mvninstall 1m 1s the patch passed
        +1 compile 0m 57s the patch passed
        +1 javac 0m 57s the patch passed
        +1 checkstyle 0m 30s the patch passed
        +1 mvnsite 1m 4s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 2m 16s the patch passed
        +1 javadoc 1m 4s the patch passed
        +1 unit 71m 14s hadoop-hdfs in the patch passed.
        +1 asflicense 0m 22s The patch does not generate ASF License warnings.
        92m 52s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-10599
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827834/HDFS-10599.001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 47ac42a611dc 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / cba973f
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16693/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16693/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 12s trunk passed +1 compile 0m 49s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 46s trunk passed +1 javadoc 1m 2s trunk passed +1 mvninstall 1m 1s the patch passed +1 compile 0m 57s the patch passed +1 javac 0m 57s the patch passed +1 checkstyle 0m 30s the patch passed +1 mvnsite 1m 4s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 16s the patch passed +1 javadoc 1m 4s the patch passed +1 unit 71m 14s hadoop-hdfs in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 92m 52s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10599 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12827834/HDFS-10599.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 47ac42a611dc 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cba973f Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16693/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16693/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        manojg Manoj Govindassamy added a comment -

        Attaching v001 patch

        • Updated DiskBalancerCLI constructor to carry a custom PrintStream
        • TestDiskBalancerCommand now runs command via ToolRunner interface
        • Added a new test TestDiskBalancerCommand#testReportWithGenericOptionFS to verify the generic option "fs" handled by diskbalancer command
        • Filed HDFS-10852 to track tests needed for HDFS script classname verification
        Show
        manojg Manoj Govindassamy added a comment - Attaching v001 patch Updated DiskBalancerCLI constructor to carry a custom PrintStream TestDiskBalancerCommand now runs command via ToolRunner interface Added a new test TestDiskBalancerCommand#testReportWithGenericOptionFS to verify the generic option "fs" handled by diskbalancer command Filed HDFS-10852 to track tests needed for HDFS script classname verification
        Hide
        manojg Manoj Govindassamy added a comment -

        Sure Anu Engineer, will tackle hdfs script correctness as part of a new bug.

        Show
        manojg Manoj Govindassamy added a comment - Sure Anu Engineer , will tackle hdfs script correctness as part of a new bug.
        Hide
        anu Anu Engineer added a comment -

        Manoj Govindassamy However we would still miss the case of "hdfs diskbalancer" – like the case where you renamed the class.
        It is an outlier and we don't have to fix that issue with this JIRA. Maybe we should file a different JIRA for that ?

        Show
        anu Anu Engineer added a comment - Manoj Govindassamy However we would still miss the case of "hdfs diskbalancer" – like the case where you renamed the class. It is an outlier and we don't have to fix that issue with this JIRA. Maybe we should file a different JIRA for that ?
        Hide
        anu Anu Engineer added a comment -

        Manoj Govindassamy That sounds like a plan. cc: Arpit Agarwal and Xiaobing Zhou for comments.

        Show
        anu Anu Engineer added a comment - Manoj Govindassamy That sounds like a plan. cc: Arpit Agarwal and Xiaobing Zhou for comments.
        Hide
        manojg Manoj Govindassamy added a comment -

        Got it. I referred how other CLI/org.apache.hadoop.util.Tool implementers (like DFSAdmin, DFSHAAdmin, DFSck, DistCp, etc.,) have their unit tests written and it looks like almost all of these are directly invoking ToolRunner.run() directly from the tests instead of using ProcessBuilder or Runtime to launch the Tool application. The main reason for running these Tools via ToolRunner directly is for submitting a custom PrintStream object (when constructing the Tool) so that Tool outputs can be verified by the test.

        Many of TestDiskBalancerCommand unit tests also need DiskBalancer CLI Tool's output for verifying messages and other expected patterns. So, looks like we can also follow the same pattern and call ToolRunner.run() with custom DiskBalancer object constructed with our own PrintStream. I prototyped this and the test runs fine.

        Anu Engineer, your thoughts on the above model please ?

        Show
        manojg Manoj Govindassamy added a comment - Got it. I referred how other CLI/org.apache.hadoop.util. Tool implementers (like DFSAdmin, DFSHAAdmin, DFSck, DistCp, etc.,) have their unit tests written and it looks like almost all of these are directly invoking ToolRunner.run() directly from the tests instead of using ProcessBuilder or Runtime to launch the Tool application. The main reason for running these Tools via ToolRunner directly is for submitting a custom PrintStream object (when constructing the Tool) so that Tool outputs can be verified by the test. Many of TestDiskBalancerCommand unit tests also need DiskBalancer CLI Tool's output for verifying messages and other expected patterns. So, looks like we can also follow the same pattern and call ToolRunner.run() with custom DiskBalancer object constructed with our own PrintStream . I prototyped this and the test runs fine. Anu Engineer , your thoughts on the above model please ?
        Hide
        anu Anu Engineer added a comment -

        Manoj Govindassamy Thanks for offering to take up this work. The issue is as follows. if you look at TestDiskBalancerCommand.java, you will see that it runs all commands using a function called runCommand, which in turn invokes runCommandInternal.

        runCommandInternal calls into org.apache.hadoop.hdfs.tools.DiskBalancer directly. However, there is a set of magic that happens in the main of org.apache.hadoop.hdfs.tools.DiskBalancer class, which is bypassed in tests. The main invokes res = ToolRunner.run(shell, argv);. This provides such options as -fs, so currently the DiskBalancerCommand tests do not exercise the ToolRunner classes when the test code is run. This creates an imperfect test environment for the command line. Executing the main of org.apache.hadoop.hdfs.tools.DiskBalancer in runCommandInternal is the objective of this JIRA.

        Please feel free to reach out to me if you need any more details.
        CC: Xiaobing Zhou to make sure that my explanation is correct.

        Show
        anu Anu Engineer added a comment - Manoj Govindassamy Thanks for offering to take up this work. The issue is as follows. if you look at TestDiskBalancerCommand.java , you will see that it runs all commands using a function called runCommand, which in turn invokes runCommandInternal . runCommandInternal calls into org.apache.hadoop.hdfs.tools.DiskBalancer directly. However, there is a set of magic that happens in the main of org.apache.hadoop.hdfs.tools.DiskBalancer class, which is bypassed in tests. The main invokes res = ToolRunner.run(shell, argv); . This provides such options as -fs , so currently the DiskBalancerCommand tests do not exercise the ToolRunner classes when the test code is run. This creates an imperfect test environment for the command line. Executing the main of org.apache.hadoop.hdfs.tools.DiskBalancer in runCommandInternal is the objective of this JIRA. Please feel free to reach out to me if you need any more details. CC: Xiaobing Zhou to make sure that my explanation is correct.
        Hide
        manojg Manoj Govindassamy added a comment -

        Based on discussion with Anu Engineer, taking up this task.

        Show
        manojg Manoj Govindassamy added a comment - Based on discussion with Anu Engineer , taking up this task.

          People

          • Assignee:
            manojg Manoj Govindassamy
            Reporter:
            anu Anu Engineer
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development