Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-907

Add tests for getBlockLocations and totalLoad metrics.

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.20.2
    • Fix Version/s: 0.20.2, 0.21.0
    • Component/s: namenode
    • Labels:
      None
    • Environment:

      This jira will add more tests for metrics reported by NameNode . numGetBlockLocations and totalLoad.

    • Hadoop Flags:
      Reviewed
    1. HDFS-907v3.patch
      6 kB
      Ravi Phulari
    2. HDFS-907v3.patch
      6 kB
      Ravi Phulari
    3. HDFS907s.patch
      6 kB
      Ravi Phulari
    4. HDFS-907-0.20.patch
      6 kB
      Ravi Phulari
    5. HDFS-907.v2.patch
      5 kB
      Ravi Phulari
    6. HDFS-907.patch
      4 kB
      Ravi Phulari

      Activity

      Hide
      Ravi Phulari added a comment -

      Attached first working patch.
      Will update this patch soon with tests for other NameNode metrics and fix for indentation errors.

      Show
      Ravi Phulari added a comment - Attached first working patch. Will update this patch soon with tests for other NameNode metrics and fix for indentation errors.
      Hide
      Konstantin Boudnik added a comment -

      A couple of comments:

      • method updateNNMetrics() sleeps for 1 second. The comment says that it happens according to dfs.replication.interval. Why don't you retrieve the actual value of this configuration parameter?
      • use some meaningful messages for the asserts
      • Thread.sleep() might throw InterruptedException which will terminate {{ updateNNMetrics()}}. Which in turn will terminate testGetBlockLocations(). Is it intended?
      • would suggest to call the test testGetBlockLocationsMetric()
      • all public methods have to have proper JavaDoc with all required tags. In this case @throws is missing

      And some nits:

      • identification suppose to be equal to 2 white spaces exactly
      • I think very last empty line is excessive
      • test method declaration doesn't have a white in throws Exception{
      • you have two empty lines modifications in
           }
        -  
        +
           private MiniDFSCluster cluster;
        

        and

           private FSNamesystem namesystem;
        -
        +  private NameNodeMetrics nnMetrics;
        +  private NameNode nn;
        +  
           private static Path getTestPath(String fileName) {
        
      Show
      Konstantin Boudnik added a comment - A couple of comments: method updateNNMetrics() sleeps for 1 second. The comment says that it happens according to dfs.replication.interval . Why don't you retrieve the actual value of this configuration parameter? use some meaningful messages for the asserts Thread.sleep() might throw InterruptedException which will terminate {{ updateNNMetrics()}}. Which in turn will terminate testGetBlockLocations() . Is it intended? would suggest to call the test testGetBlockLocationsMetric() all public methods have to have proper JavaDoc with all required tags. In this case @throws is missing And some nits: identification suppose to be equal to 2 white spaces exactly I think very last empty line is excessive test method declaration doesn't have a white in throws Exception{ you have two empty lines modifications in } - + private MiniDFSCluster cluster; and private FSNamesystem namesystem; - + private NameNodeMetrics nnMetrics; + private NameNode nn; + private static Path getTestPath(String fileName) {
      Hide
      Konstantin Boudnik added a comment -

      Oh, one more: the JIRA's title says "Add ... unit tests". These aren't unit tests. So please consider writing true unit tests, e.g. using Mockito, or change the title of the JIRA.

      Show
      Konstantin Boudnik added a comment - Oh, one more: the JIRA's title says "Add ... unit tests". These aren't unit tests. So please consider writing true unit tests, e.g. using Mockito, or change the title of the JIRA.
      Hide
      Ravi Phulari added a comment -

      Cos, thanks for reviewing patch and useful comments and suggestions.
      Uploading new patch updated as per your suggestions.
      Thanks.

      Show
      Ravi Phulari added a comment - Cos, thanks for reviewing patch and useful comments and suggestions. Uploading new patch updated as per your suggestions. Thanks.
      Hide
      Konstantin Boudnik added a comment -

      I think I like what I see. One more thing: please add some meaningful message to the assertion statements - it will easy the fault analysis effort.

      Show
      Konstantin Boudnik added a comment - I think I like what I see. One more thing: please add some meaningful message to the assertion statements - it will easy the fault analysis effort.
      Hide
      Konstantin Boudnik added a comment -

      More to go, Ravi

      +    // and previous interval are 0
      +    assertEquals(0,nnMetrics.numGetBlockLocations.getPreviousIntervalValue());
      +    assertEquals(0,nnMetrics.numGetBlockLocations.getCurrentIntervalValue());
      

      BTW, you don't need to track versions of your patches manually (e.g. v1, v2, etc.): simply keep attaching the file with the same name and JIRA will do the rest. No patches will be lost either.

      Show
      Konstantin Boudnik added a comment - More to go, Ravi + // and previous interval are 0 + assertEquals(0,nnMetrics.numGetBlockLocations.getPreviousIntervalValue()); + assertEquals(0,nnMetrics.numGetBlockLocations.getCurrentIntervalValue()); BTW, you don't need to track versions of your patches manually (e.g. v1, v2, etc.): simply keep attaching the file with the same name and JIRA will do the rest. No patches will be lost either.
      Hide
      Ravi Phulari added a comment -

      Good catch Cos. Duhhh lazy me .
      Attaching updated patch.

      Show
      Ravi Phulari added a comment - Good catch Cos. Duhhh lazy me . Attaching updated patch.
      Hide
      Konstantin Boudnik added a comment -

      +1 patch looks good. Since this is for 0.20 please run test-patch locally and post the results here.

      Show
      Konstantin Boudnik added a comment - +1 patch looks good. Since this is for 0.20 please run test-patch locally and post the results here.
      Hide
      Hadoop QA added a comment -

      -1 overall. Here are the results of testing the latest attachment
      http://issues.apache.org/jira/secure/attachment/12431853/HDFS-907v3.patch
      against trunk revision 903906.

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

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

      -1 contrib tests. The patch failed contrib unit tests.

      Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/testReport/
      Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/artifact/trunk/build/test/checkstyle-errors.html
      Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/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/12431853/HDFS-907v3.patch against trunk revision 903906. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/console This message is automatically generated.
      Hide
      Ravi Phulari added a comment -

      Above test failures (core and contrib) are not related to patch. There is something terribly wrong going on with Patch reporting.
      I say so because *Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/testReport/* says that there are no test failures but above comment posts that there are failures.

      Show
      Ravi Phulari added a comment - Above test failures (core and contrib) are not related to patch. There is something terribly wrong going on with Patch reporting. I say so because *Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/109/testReport/* says that there are no test failures but above comment posts that there are failures.
      Hide
      Ravi Phulari added a comment -

      Attaching patch for 0.20 branch.
      test-patch passed successfully , pasting test-patch result log.

      [exec] 
           [exec] 
           [exec] +1 overall.  
           [exec] 
           [exec]     +1 @author.  The patch does not contain any @author tags.
           [exec] 
           [exec]     +1 tests included.  The patch appears to include 3 new or modified tests.
           [exec] 
           [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
           [exec] 
           [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
           [exec] 
           [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
           [exec] 
           [exec]     +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
           [exec] 
           [exec] 
           [exec] 
           [exec] 
           [exec] ======================================================================
           [exec] ======================================================================
           [exec]     Finished build.
           [exec] ======================================================================
           [exec] ======================================================================
           [exec] 
           [exec] 
      
      BUILD SUCCESSFUL
      Total time: 22 minutes 27 seconds
      
      
      Show
      Ravi Phulari added a comment - Attaching patch for 0.20 branch. test-patch passed successfully , pasting test-patch result log. [exec] [exec] [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ====================================================================== [exec] [exec] BUILD SUCCESSFUL Total time: 22 minutes 27 seconds
      Hide
      Ravi Phulari added a comment -

      Attaching patch for Yahoo! distribution.

      Show
      Ravi Phulari added a comment - Attaching patch for Yahoo! distribution.
      Hide
      Konstantin Boudnik added a comment -

      I've just committed this into two unreleased branches in trunk. Thank you Ravi.

      Show
      Konstantin Boudnik added a comment - I've just committed this into two unreleased branches in trunk. Thank you Ravi.
      Hide
      Ravi Phulari added a comment -

      Thanks for committing Cos.

      On 2/4/10 8:10 PM, "Konstantin Boudnik (JIRA)" <jira@apache.org> wrote:

      [ https://issues.apache.org/jira/browse/HDFS-907?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

      Konstantin Boudnik updated HDFS-907:
      ------------------------------------

      Resolution: Fixed
      Fix Version/s: 0.22.0
      0.21.0
      Hadoop Flags: [Reviewed]
      Status: Resolved (was: Patch Available)

      I've just committed this into two unreleased branches in trunk. Thank you Ravi.


      This message is automatically generated by JIRA.
      -
      You can reply to this email to add a comment to the issue online.

      Ravi

      Show
      Ravi Phulari added a comment - Thanks for committing Cos. On 2/4/10 8:10 PM, "Konstantin Boudnik (JIRA)" <jira@apache.org> wrote: [ https://issues.apache.org/jira/browse/HDFS-907?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Konstantin Boudnik updated HDFS-907 : ------------------------------------ Resolution: Fixed Fix Version/s: 0.22.0 0.21.0 Hadoop Flags: [Reviewed] Status: Resolved (was: Patch Available) I've just committed this into two unreleased branches in trunk. Thank you Ravi. – This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online. Ravi –
      Hide
      Hudson added a comment -

      Integrated in Hadoop-Hdfs-trunk-Commit #191 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/191/)

      Show
      Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #191 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/191/ )
      Hide
      Hudson added a comment -

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

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

      Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/)

      Show
      Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/ )

        People

        • Assignee:
          Ravi Phulari
          Reporter:
          Ravi Phulari
        • Votes:
          0 Vote for this issue
          Watchers:
          2 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development