HBase
  1. HBase
  2. HBASE-4348

Add metrics for regions in transition

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.92.0
    • Fix Version/s: 0.95.0
    • Component/s: metrics
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      The following metrics would be useful for monitoring the master:

      • the number of regions in transition
      • the number of regions in transition that have been in transition for more than a minute
      • how many seconds has the oldest region-in-transition been in transition
      1. 4348-metrics-v3.patch
        10 kB
        Himanshu Vashishtha
      2. 4348-v1.patch
        3 kB
        Himanshu Vashishtha
      3. 4348-v2.patch
        3 kB
        Himanshu Vashishtha
      4. hbase-4348.patch
        15 kB
        Himanshu Vashishtha
      5. hbase-4348-v4.patch
        15 kB
        Jonathan Hsieh
      6. metrics-v2.patch
        10 kB
        Himanshu Vashishtha
      7. RegionInTransitions2.png
        154 kB
        Himanshu Vashishtha
      8. RITs.png
        42 kB
        Himanshu Vashishtha

        Activity

        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        Hide
        Himanshu Vashishtha added a comment -

        The failed test (org.apache.hadoop.hbase.coprocessor.TestRowProcessorEndpoint) passes locally for me, though.

        Show
        Himanshu Vashishtha added a comment - The failed test (org.apache.hadoop.hbase.coprocessor.TestRowProcessorEndpoint) passes locally for me, though.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #160 (See https://builds.apache.org/job/HBase-TRUNK-security/160/)
        HBASE-4348 Add metrics for regions in transition (Himanshu Vashishtha) (Revision 1310159)

        Result = FAILURE
        jmhsieh :
        Files :

        • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #160 (See https://builds.apache.org/job/HBase-TRUNK-security/160/ ) HBASE-4348 Add metrics for regions in transition (Himanshu Vashishtha) (Revision 1310159) Result = FAILURE jmhsieh : Files : /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2718 (See https://builds.apache.org/job/HBase-TRUNK/2718/)
        HBASE-4348 Add metrics for regions in transition (Himanshu Vashishtha) (Revision 1310159)

        Result = SUCCESS
        jmhsieh :
        Files :

        • /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2718 (See https://builds.apache.org/job/HBase-TRUNK/2718/ ) HBASE-4348 Add metrics for regions in transition (Himanshu Vashishtha) (Revision 1310159) Result = SUCCESS jmhsieh : Files : /hbase/trunk/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Hide
        Jonathan Hsieh added a comment -

        The failed test has been flakey recently.

        Committed. Thanks Himanshu and thanks Ted for reviews.

        Show
        Jonathan Hsieh added a comment - The failed test has been flakey recently. Committed. Thanks Himanshu and thanks Ted for reviews.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521634/hbase-4348-v4.patch
        against trunk revision .

        +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 (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 unit tests:
        org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1420//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1420//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1420//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/12521634/hbase-4348-v4.patch against trunk revision . +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 (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 unit tests: org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1420//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1420//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1420//console This message is automatically generated.
        Hide
        Jonathan Hsieh added a comment -

        minor fix to deal with conflict

        Show
        Jonathan Hsieh added a comment - minor fix to deal with conflict
        Hide
        Jonathan Hsieh added a comment -

        You probably need to update/fetch again. trunk lives in svn and has a different style for commit numbers. The hash you gave is git – if you are using git.apache.org it often lags and doesn't have the most recent patches that svn has.

        Looks like the conflict was with "HBASE-5715 Revert 'Instant schema alter' for now, HBASE-4213". It's minor so I've updated the patch. We'll let the bot test the slightly updated version and if it is happy I'll commit.

        Show
        Jonathan Hsieh added a comment - You probably need to update/fetch again. trunk lives in svn and has a different style for commit numbers. The hash you gave is git – if you are using git.apache.org it often lags and doesn't have the most recent patches that svn has. Looks like the conflict was with " HBASE-5715 Revert 'Instant schema alter' for now, HBASE-4213 ". It's minor so I've updated the patch. We'll let the bot test the slightly updated version and if it is happy I'll commit.
        Hide
        Himanshu Vashishtha added a comment -

        I tested it with latest trunk commit hash (f3168b76f48541ce89e88016b333c0d66fb12f40) and it applies good.

        Is hadoop qa is applying it some other branch? 0.92?

        Show
        Himanshu Vashishtha added a comment - I tested it with latest trunk commit hash (f3168b76f48541ce89e88016b333c0d66fb12f40) and it applies good. Is hadoop qa is applying it some other branch? 0.92?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521607/hbase-4348.patch
        against trunk revision .

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1418//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/12521607/hbase-4348.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1418//console This message is automatically generated.
        Hide
        Himanshu Vashishtha added a comment -

        Patch after reviews.

        Show
        Himanshu Vashishtha added a comment - Patch after reviews.
        Hide
        Ted Yu added a comment -

        I don't have other comments.

        Show
        Ted Yu added a comment - I don't have other comments.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6703
        -----------------------------------------------------------

        Ship it!

        I'm happy with this. Ted do you have comments? If no, I'll commit in about 24 hours.

        src/main/java/org/apache/hadoop/hbase/HConstants.java
        <https://reviews.apache.org/r/4402/#comment14507>

        I like this name.

        • jmhsieh

        On 2012-04-04 21:47:04, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-04-04 21:47:04)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6703 ----------------------------------------------------------- Ship it! I'm happy with this. Ted do you have comments? If no, I'll commit in about 24 hours. src/main/java/org/apache/hadoop/hbase/HConstants.java < https://reviews.apache.org/r/4402/#comment14507 > I like this name. jmhsieh On 2012-04-04 21:47:04, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-04-04 21:47:04) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-04 01:55:42, Himanshu Vashishtha wrote:

        > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 655

        > <https://reviews.apache.org/r/4402/diff/6/?file=97548#file97548line655>

        >

        > Jon: This is not a refresh metrics per se. Its for defining what a user considers to be a longer period for a Region to be in a transition state, and show these regions count as a metric.

        > So, it can be hbase.metrics.rit.stuck.warning.threshold?

        > Long name, though. Please let me know amd I will change accordingly and revise the patch.

        got it.

        • jmhsieh

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6669
        -----------------------------------------------------------

        On 2012-04-04 21:47:04, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-04-04 21:47:04)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-04 01:55:42, Himanshu Vashishtha wrote: > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 655 > < https://reviews.apache.org/r/4402/diff/6/?file=97548#file97548line655 > > > Jon: This is not a refresh metrics per se. Its for defining what a user considers to be a longer period for a Region to be in a transition state, and show these regions count as a metric. > So, it can be hbase.metrics.rit.stuck.warning.threshold? > Long name, though. Please let me know amd I will change accordingly and revise the patch. got it. jmhsieh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6669 ----------------------------------------------------------- On 2012-04-04 21:47:04, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-04-04 21:47:04) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-04-04 21:47:04.382172)

        Review request for hbase.

        Changes
        -------

        Changed the name of the property for defining the threshold time limit to show the Region in transitions in UI; ans other nits from Ted.

        Summary
        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-04-04 21:47:04.382172) Review request for hbase. Changes ------- Changed the name of the property for defining the threshold time limit to show the Region in transitions in UI; ans other nits from Ted. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6669
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/HConstants.java
        <https://reviews.apache.org/r/4402/#comment14427>

        Jon: This is not a refresh metrics per se. Its for defining what a user considers to be a longer period for a Region to be in a transition state, and show these regions count as a metric.
        So, it can be hbase.metrics.rit.stuck.warning.threshold?
        Long name, though. Please let me know amd I will change accordingly and revise the patch.

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14428>

        They are single line if statement, but will do.

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14429>

        Not following, sorry. Please explain.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment14430>

        ok, will do.

        • Himanshu

        On 2012-03-30 05:21:12, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-30 05:21:12)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6669 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/HConstants.java < https://reviews.apache.org/r/4402/#comment14427 > Jon: This is not a refresh metrics per se. Its for defining what a user considers to be a longer period for a Region to be in a transition state, and show these regions count as a metric. So, it can be hbase.metrics.rit.stuck.warning.threshold? Long name, though. Please let me know amd I will change accordingly and revise the patch. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14428 > They are single line if statement, but will do. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14429 > Not following, sorry. Please explain. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment14430 > ok, will do. Himanshu On 2012-03-30 05:21:12, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-30 05:21:12) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        Jonathan Hsieh added a comment -

        @Otis I generally like the policy of setting versions on commit, or having a release manager set it if they decide it is necessary for a release.

        Show
        Jonathan Hsieh added a comment - @Otis I generally like the policy of setting versions on commit, or having a release manager set it if they decide it is necessary for a release.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-04-01 15:13:59, Ted Yu wrote:

        > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 655

        > <https://reviews.apache.org/r/4402/diff/6/?file=97548#file97548line655>

        >

        > I think the trailing '.time' isn't needed. Take a look at existing config parameter names involving threshold:

        >

        bq.  >        this.thresholdIdleConnections = conf.getInt("ipc.client.idlethreshold", 4000);
        bq.  >     src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        bq.  >             "putsortreducer.row.threshold", 2L * (1<<30));
        bq.  >     src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java
        bq.  >     

        >

        Actually, looking at the metric name without context, hbase.metrics.rit.threshold makes me think this is a count of the number of max regions in transition. With the .time suffix, it makes me think it is the max time for an RIT which also isn't quite right. If all things are in millis than we probably don't need units but it doesn't hurt IMO. What do you think of something like: "hbase.metrics.rit.refresh.millis", "hbase.metrics.rit.refresh.threshold.millis", or "hbase.metrics.rit.refresh.threshold"?

        • jmhsieh

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6604
        -----------------------------------------------------------

        On 2012-03-30 05:21:12, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-30 05:21:12)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-04-01 15:13:59, Ted Yu wrote: > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 655 > < https://reviews.apache.org/r/4402/diff/6/?file=97548#file97548line655 > > > I think the trailing '.time' isn't needed. Take a look at existing config parameter names involving threshold: > bq. > this .thresholdIdleConnections = conf.getInt( "ipc.client.idlethreshold" , 4000); bq. > src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java bq. > "putsortreducer.row.threshold" , 2L * (1<<30)); bq. > src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java bq. > > Actually, looking at the metric name without context, hbase.metrics.rit.threshold makes me think this is a count of the number of max regions in transition. With the .time suffix, it makes me think it is the max time for an RIT which also isn't quite right. If all things are in millis than we probably don't need units but it doesn't hurt IMO. What do you think of something like: "hbase.metrics.rit.refresh.millis", "hbase.metrics.rit.refresh.threshold.millis", or "hbase.metrics.rit.refresh.threshold"? jmhsieh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6604 ----------------------------------------------------------- On 2012-03-30 05:21:12, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-30 05:21:12) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6604
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/HConstants.java
        <https://reviews.apache.org/r/4402/#comment14269>

        I think the trailing '.time' isn't needed. Take a look at existing config parameter names involving threshold:

               this.thresholdIdleConnections = conf.getInt("ipc.client.idlethreshold", 4000);
            src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
                    "putsortreducer.row.threshold", 2L * (1<<30));
            src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java
            

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14270>

        Please add curly braces around the following line.

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14271>

        Lift this line to line 2733.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment14272>

        'out' isn't needed here. It would be nice to combine this sentence into the comment for this method.

        • Ted

        On 2012-03-30 05:21:12, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-30 05:21:12)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6604 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/HConstants.java < https://reviews.apache.org/r/4402/#comment14269 > I think the trailing '.time' isn't needed. Take a look at existing config parameter names involving threshold: this .thresholdIdleConnections = conf.getInt( "ipc.client.idlethreshold" , 4000); src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java "putsortreducer.row.threshold" , 2L * (1<<30)); src/main/java/org/apache/hadoop/hbase/mapreduce/PutSortReducer.java src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14270 > Please add curly braces around the following line. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14271 > Lift this line to line 2733. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment14272 > 'out' isn't needed here. It would be nice to combine this sentence into the comment for this method. Ted On 2012-03-30 05:21:12, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-30 05:21:12) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        Otis Gospodnetic added a comment -

        Fix Version/s is set to None. Is this for 0.96?

        Show
        Otis Gospodnetic added a comment - Fix Version/s is set to None. Is this for 0.96?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-03-30 05:21:12.616277)

        Review request for hbase.

        Changes
        -------

        Changes done as per Jon and Ted's comments. Thanks.

        Summary
        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-30 05:21:12.616277) Review request for hbase. Changes ------- Changes done as per Jon and Ted's comments. Thanks. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 21ac4ba src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 64def15 src/main/java/org/apache/hadoop/hbase/master/HMaster.java 9bd4ace src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 91dce36 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        Jonathan Hsieh added a comment -

        Himanshu, for us to commit patches, we need you to attach the patch to the JIRA and the grant permission. Currently, review board is optional, but makes it way easier for us to review.

        Please do so after you address the latest comments. Thanks!

        Show
        Jonathan Hsieh added a comment - Himanshu, for us to commit patches, we need you to attach the patch to the JIRA and the grant permission. Currently, review board is optional, but makes it way easier for us to review. Please do so after you address the latest comments. Thanks!
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6540
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14206>

        Ok, but what is the rationale behind having such a metrics?

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment14205>

        Ok. I will rename it.

        • Himanshu

        On 2012-03-29 15:57:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-29 15:57:19)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6540 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14206 > Ok, but what is the rationale behind having such a metrics? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment14205 > Ok. I will rename it. Himanshu On 2012-03-29 15:57:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-29 15:57:19) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6538
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14202>

        Can we have a metric for how long this routine takes ?

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment14203>

        lastMsg makes me think of the last message.
        How about naming this variable lastMsgTs or something similar ?

        • Ted

        On 2012-03-29 15:57:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-29 15:57:19)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6538 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14202 > Can we have a metric for how long this routine takes ? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment14203 > lastMsg makes me think of the last message. How about naming this variable lastMsgTs or something similar ? Ted On 2012-03-29 15:57:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-29 15:57:19) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6537
        -----------------------------------------------------------

        Ship it!

        lgtm. I'll fix the spacing nits as I commit.

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment14199>

        nit: spacing

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment14200>

        spacing

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment14201>

        spacing

        • jmhsieh

        On 2012-03-29 15:57:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-29 15:57:19)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6537 ----------------------------------------------------------- Ship it! lgtm. I'll fix the spacing nits as I commit. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment14199 > nit: spacing src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment14200 > spacing src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment14201 > spacing jmhsieh On 2012-03-29 15:57:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-29 15:57:19) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-03-29 15:57:19.858937)

        Review request for hbase.

        Changes
        -------

        changes done as per Jon's feedback.

        The access to MasterMetrics instance in the AssignmentManager class is guarded by a null check.

        Summary
        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-29 15:57:19.858937) Review request for hbase. Changes ------- changes done as per Jon's feedback. The access to MasterMetrics instance in the AssignmentManager class is guarded by a null check. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/HConstants.java 19be4de src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6204
        -----------------------------------------------------------

        Himanshu, please address the potential NPE issue. I've added some suggestions to keep names consistent with HBase's conventions.

        It would be really nice if you could do a test that would exercise some of the new code (test updates don't really seem do it). See TestRpcMetrics, or TestMetricsMBeanBase as possible modes. I won't block committing if this doesn't happen, but it would be nice.

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon
        <https://reviews.apache.org/r/4402/#comment13873>

        let's rename this to be consistent with other Configuration fields. Check out HConstants.java to see the names of quite a few configuration variables to get a general idea of the pattern.

        My suggestion is something like:
        'hbase.metrics.rit.threshold.time'

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment13857>

        Either:

        • javadoc to say this must not be null and add
          'Preconditions.assertNotNull(masterMetrics,"master metrics should never be null") on initialization
        • add guards where masterMetrics is deref'ed to see if null.

        Seems like with your tests, adding the guard 'if !=null' guard to masterMetrics derefs in the next comment is easier.

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment13874>

        ditto. Actually, since it is used in a few places, you should probably to add this to the HConstants file.

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment13858>

        master metrics could npe here.

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment13875>

        nit: spitting? (kind gross) maybe "emitting" (you use that word below) or "publishing"?

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        <https://reviews.apache.org/r/4402/#comment13363>

        nit: funny spacing

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java
        <https://reviews.apache.org/r/4402/#comment13876>

        maybe rename to put rit in front so that it is consistent and will sort nicely?

        maybe 'ritOldestAge'?

        • jmhsieh

        On 2012-03-20 21:44:10, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-20 21:44:10)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6204 ----------------------------------------------------------- Himanshu, please address the potential NPE issue. I've added some suggestions to keep names consistent with HBase's conventions. It would be really nice if you could do a test that would exercise some of the new code (test updates don't really seem do it). See TestRpcMetrics, or TestMetricsMBeanBase as possible modes. I won't block committing if this doesn't happen, but it would be nice. src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon < https://reviews.apache.org/r/4402/#comment13873 > let's rename this to be consistent with other Configuration fields. Check out HConstants.java to see the names of quite a few configuration variables to get a general idea of the pattern. My suggestion is something like: 'hbase.metrics.rit.threshold.time' src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment13857 > Either: javadoc to say this must not be null and add 'Preconditions.assertNotNull(masterMetrics,"master metrics should never be null") on initialization add guards where masterMetrics is deref'ed to see if null. Seems like with your tests, adding the guard 'if !=null' guard to masterMetrics derefs in the next comment is easier. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment13874 > ditto. Actually, since it is used in a few places, you should probably to add this to the HConstants file. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment13858 > master metrics could npe here. src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment13875 > nit: spitting? (kind gross) maybe "emitting" (you use that word below) or "publishing"? src/main/java/org/apache/hadoop/hbase/master/HMaster.java < https://reviews.apache.org/r/4402/#comment13363 > nit: funny spacing src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java < https://reviews.apache.org/r/4402/#comment13876 > maybe rename to put rit in front so that it is consistent and will sort nicely? maybe 'ritOldestAge'? jmhsieh On 2012-03-20 21:44:10, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-20 21:44:10) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-03-20 21:44:10.091442)

        Review request for hbase.

        Changes
        -------

        Thanks Greg; removed the extra braces

        Summary
        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-20 21:44:10.091442) Review request for hbase. Changes ------- Thanks Greg; removed the extra braces Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6144
        -----------------------------------------------------------

        lgtm

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon
        <https://reviews.apache.org/r/4402/#comment13225>

        nit: extra parens

        • Gregory

        On 2012-03-20 21:14:04, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-20 21:14:04)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6144 ----------------------------------------------------------- lgtm src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon < https://reviews.apache.org/r/4402/#comment13225 > nit: extra parens Gregory On 2012-03-20 21:14:04, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-20 21:14:04) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-03-20 21:14:04.093757)

        Review request for hbase.

        Changes
        -------

        Thanks for the review Greg.
        Updated to have a "OverThreshold" prefix.

        Summary
        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-20 21:14:04.093757) Review request for hbase. Changes ------- Thanks for the review Greg. Updated to have a "OverThreshold" prefix. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6134
        -----------------------------------------------------------

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon
        <https://reviews.apache.org/r/4402/#comment13195>

        This should be called OverThreshold, not MoreThanAMin.

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon
        <https://reviews.apache.org/r/4402/#comment13198>

        This should be say something like "more than X milliseconds"

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment13200>

        Same here.

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java
        <https://reviews.apache.org/r/4402/#comment13201>

        Here too.

        • Gregory

        On 2012-03-20 07:03:17, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-20 07:03:17)

        Review request for hbase.

        Summary

        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6134 ----------------------------------------------------------- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon < https://reviews.apache.org/r/4402/#comment13195 > This should be called OverThreshold, not MoreThanAMin. src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon < https://reviews.apache.org/r/4402/#comment13198 > This should be say something like "more than X milliseconds" src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment13200 > Same here. src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java < https://reviews.apache.org/r/4402/#comment13201 > Here too. Gregory On 2012-03-20 07:03:17, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-20 07:03:17) Review request for hbase. Summary ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-03-20 07:03:17.121941)

        Review request for hbase.

        Changes
        -------

        Incorporated Greg's comments about formatting.
        Added a new property for RIT metrics threshold "rit.metrics.threshold.time" and its default is 60000 (ms).

        Summary (updated)
        -------

        This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs (updated)


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-20 07:03:17.121941) Review request for hbase. Changes ------- Incorporated Greg's comments about formatting. Added a new property for RIT metrics threshold "rit.metrics.threshold.time" and its default is 60000 (ms). Summary (updated) ------- This patch is for adding Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs (updated) src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-19 19:05:08, Gregory Chanan wrote:

        > Looks pretty good, just some spacing issues.

        >

        > Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one).

        Himanshu Vashishtha wrote:

        Thanks for the review. I will make these changes and revise the patch.

        Himanshu Vashishtha wrote:

        Note that 60 seconds thing is actually used from the jira description, and is not using any property as such. I can make this configurable though.

        The region server property (hbase.regionserver.msginterval, default is 3 sec) which i used is for the frequency for emitting out the metrics. Should that be different for Master and RS?

        I know, I was just asking a question because I don't have much operational experience. If you or others think 60 seconds is a good cutoff and it doesn't need to be configurable, that sounds good to me.
        If you are not going to make it a property, you should only have it calculated in one place so it is easy to change .

        • Gregory

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6076
        -----------------------------------------------------------

        On 2012-03-19 06:48:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-19 06:48:19)

        Review request for hbase.

        Summary

        -------

        This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-19 19:05:08, Gregory Chanan wrote: > Looks pretty good, just some spacing issues. > > Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one). Himanshu Vashishtha wrote: Thanks for the review. I will make these changes and revise the patch. Himanshu Vashishtha wrote: Note that 60 seconds thing is actually used from the jira description, and is not using any property as such. I can make this configurable though. The region server property (hbase.regionserver.msginterval, default is 3 sec) which i used is for the frequency for emitting out the metrics. Should that be different for Master and RS? I know, I was just asking a question because I don't have much operational experience. If you or others think 60 seconds is a good cutoff and it doesn't need to be configurable, that sounds good to me. If you are not going to make it a property, you should only have it calculated in one place so it is easy to change . Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6076 ----------------------------------------------------------- On 2012-03-19 06:48:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-19 06:48:19) Review request for hbase. Summary ------- This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-19 19:05:08, Gregory Chanan wrote:

        > Looks pretty good, just some spacing issues.

        >

        > Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one).

        Himanshu Vashishtha wrote:

        Thanks for the review. I will make these changes and revise the patch.

        Note that 60 seconds thing is actually used from the jira description, and is not using any property as such. I can make this configurable though.
        The region server property (hbase.regionserver.msginterval, default is 3 sec) which i used is for the frequency for emitting out the metrics. Should that be different for Master and RS?

        • Himanshu

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6076
        -----------------------------------------------------------

        On 2012-03-19 06:48:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-19 06:48:19)

        Review request for hbase.

        Summary

        -------

        This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-19 19:05:08, Gregory Chanan wrote: > Looks pretty good, just some spacing issues. > > Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one). Himanshu Vashishtha wrote: Thanks for the review. I will make these changes and revise the patch. Note that 60 seconds thing is actually used from the jira description, and is not using any property as such. I can make this configurable though. The region server property (hbase.regionserver.msginterval, default is 3 sec) which i used is for the frequency for emitting out the metrics. Should that be different for Master and RS? Himanshu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6076 ----------------------------------------------------------- On 2012-03-19 06:48:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-19 06:48:19) Review request for hbase. Summary ------- This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-19 19:05:08, Gregory Chanan wrote:

        > Looks pretty good, just some spacing issues.

        >

        > Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one).

        Thanks for the review. I will make these changes and revise the patch.

        • Himanshu

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6076
        -----------------------------------------------------------

        On 2012-03-19 06:48:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-19 06:48:19)

        Review request for hbase.

        Summary

        -------

        This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-19 19:05:08, Gregory Chanan wrote: > Looks pretty good, just some spacing issues. > > Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one). Thanks for the review. I will make these changes and revise the patch. Himanshu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6076 ----------------------------------------------------------- On 2012-03-19 06:48:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-19 06:48:19) Review request for hbase. Summary ------- This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/#review6076
        -----------------------------------------------------------

        Looks pretty good, just some spacing issues.

        Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one).

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        <https://reviews.apache.org/r/4402/#comment13055>

        The spacing looks wrong here in all the java code – everywhere else in the code it looks like we use two spaces for an indent level, whereas here you are using tabs.

        Also, the braces aren't lined up.

        I don't see anything about spacing at this page, though:
        http://hbase.apache.org/book/submitting.patches.html
        Perhaps we should update it.

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java
        <https://reviews.apache.org/r/4402/#comment13056>

        From the submitting patches page:
        "Keep lines less than 80 characters."

        • Gregory

        On 2012-03-19 06:48:19, Himanshu Vashishtha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4402/

        -----------------------------------------------------------

        (Updated 2012-03-19 06:48:19)

        Review request for hbase.

        Summary

        -------

        This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.

        https://issues.apache.org/jira/browse/HBase-4348

        Diffs

        -----

        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691

        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca

        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30

        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52

        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing

        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/#review6076 ----------------------------------------------------------- Looks pretty good, just some spacing issues. Are we sure that 60 seconds is the proper timeout to display "interesting" regions in transition? Perhaps we should make this configurable? (If yes, I'd also create a master msgInterval instead of reusing the regionserver one). src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java < https://reviews.apache.org/r/4402/#comment13055 > The spacing looks wrong here in all the java code – everywhere else in the code it looks like we use two spaces for an indent level, whereas here you are using tabs. Also, the braces aren't lined up. I don't see anything about spacing at this page, though: http://hbase.apache.org/book/submitting.patches.html Perhaps we should update it. src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java < https://reviews.apache.org/r/4402/#comment13056 > From the submitting patches page: "Keep lines less than 80 characters." Gregory On 2012-03-19 06:48:19, Himanshu Vashishtha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-19 06:48:19) Review request for hbase. Summary ------- This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs ----- src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4402/
        -----------------------------------------------------------

        (Updated 2012-03-19 06:48:19.158231)

        Review request for hbase.

        Summary (updated)
        -------

        This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348.

        This addresses bug HBase-4348.
        https://issues.apache.org/jira/browse/HBase-4348

        Diffs


        src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691
        src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca
        src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30
        src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52
        src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33

        Diff: https://reviews.apache.org/r/4402/diff

        Testing
        -------

        Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean;

        mvn test passes without any failure.

        Thanks,

        Himanshu

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4402/ ----------------------------------------------------------- (Updated 2012-03-19 06:48:19.158231) Review request for hbase. Summary (updated) ------- This patch is for addinf Region in transition metrics to the HMaster metrics system. It also adds these metrics in the master ui, in the Region in transition section. I have attached the proposed new format in the jira 4348. This addresses bug HBase-4348. https://issues.apache.org/jira/browse/HBase-4348 Diffs src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon 0dc0691 src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java ae468ca src/main/java/org/apache/hadoop/hbase/master/HMaster.java c4b4d30 src/main/java/org/apache/hadoop/hbase/master/metrics/MasterMetrics.java 83abc52 src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java d68ce33 Diff: https://reviews.apache.org/r/4402/diff Testing ------- Ran on a 5 node cluster and kill region servers randomly to observe the changes in the RIT metrics as emitted out by the Master's mxbean; mvn test passes without any failure. Thanks, Himanshu
        Hide
        Himanshu Vashishtha added a comment -

        Thanks for the review Jon. I incorporated your suggestions, and have tested this new patch on a 5 node cluster and randomly killing regions servers to bring some regions in transition, and noting the change in metrics. I wonder how the metrics for RIT can be tested by some simple unit tests.

        Attached is the new UI for region in transition section on the master UI. I will upload a review request on the board soon.

        Show
        Himanshu Vashishtha added a comment - Thanks for the review Jon. I incorporated your suggestions, and have tested this new patch on a 5 node cluster and randomly killing regions servers to bring some regions in transition, and noting the change in metrics. I wonder how the metrics for RIT can be tested by some simple unit tests. Attached is the new UI for region in transition section on the master UI. I will upload a review request on the board soon.
        Hide
        Jonathan Hsieh added a comment -

        @Himanshu

        My guess is it is a perf thing – regionserver metrics include gets and puts which can be costly to count efficiently and are using a special counter library (and are thus not incremented using the hadoop Metrics library's methods). While we want these RIT metrics, I don't think performance of the counters on RIT's is something we need to worry about.

        I have some suggestions and a handful of nits from just looking at the v3 patch.

        Suggestions:

        • I think you may need to do pushMetric(metricsRecord) on the new metrics in the doUpdates method to "publish" the values?
        • Previous versions had tests but this does not - plesae add tests or combine into a single patch for review?
        • Maybe use reviews.apache.org to make it easier to do reviews?

        nits:

        • There were some typos in the last line of the png file. please check/fix spelling!
        Show
        Jonathan Hsieh added a comment - @Himanshu My guess is it is a perf thing – regionserver metrics include gets and puts which can be costly to count efficiently and are using a special counter library (and are thus not incremented using the hadoop Metrics library's methods). While we want these RIT metrics, I don't think performance of the counters on RIT's is something we need to worry about. I have some suggestions and a handful of nits from just looking at the v3 patch. Suggestions: I think you may need to do pushMetric(metricsRecord) on the new metrics in the doUpdates method to "publish" the values? Previous versions had tests but this does not - plesae add tests or combine into a single patch for review? Maybe use reviews.apache.org to make it easier to do reviews? nits: There were some typos in the last line of the png file. please check/fix spelling!
        Hide
        Himanshu Vashishtha added a comment -

        Changes done.

        Show
        Himanshu Vashishtha added a comment - Changes done.
        Hide
        Himanshu Vashishtha added a comment -

        I found one issue with the existing patch; will upload a new one soon!

        Show
        Himanshu Vashishtha added a comment - I found one issue with the existing patch; will upload a new one soon!
        Hide
        Himanshu Vashishtha added a comment -

        I have come up with the following approach: (and have attached an initial patch too)
        a) Having an "active" HMaster also emit out metrics at a fixed intervals (in align with a Region Server).
        b) Defining new metrics in MasterMetrics class, and updating them in the AssignmentManager class. This avoids calling the getRegionsInTransition method on the AM class (it creates a copy of the map). This entails adding one more parameter in the AM constructor. I changed the current constructor instead of adding one more as it was only created in the HMaster class. I needed to add null as the param in the TestAssignmentManager class (its test are passing).

        Please let me know if this approach sounds good. Once this is done, I will work on the UI to make it more colorful as suggested by Stack.

        Show
        Himanshu Vashishtha added a comment - I have come up with the following approach: (and have attached an initial patch too) a) Having an "active" HMaster also emit out metrics at a fixed intervals (in align with a Region Server). b) Defining new metrics in MasterMetrics class, and updating them in the AssignmentManager class. This avoids calling the getRegionsInTransition method on the AM class (it creates a copy of the map). This entails adding one more parameter in the AM constructor. I changed the current constructor instead of adding one more as it was only created in the HMaster class. I needed to add null as the param in the TestAssignmentManager class (its test are passing). Please let me know if this approach sounds good. Once this is done, I will work on the UI to make it more colorful as suggested by Stack.
        Hide
        Himanshu Vashishtha added a comment -

        I have done some analysis and some questions regarding the metrics here. Please correct me if I am wrong.

        RegionServer updates its metrics at fix intervals defined by "hbase.regionserver.msginterval" and the metrics are pushed to the record after specified interval.
        Master don't use this methodology; rather whenever the master statistics are changed, the caller is supposed to update the metrics value on its own.

        I am wondering why we don't follow the same approach for master (because the stats we are recording are not changing that often?). The reason I am asking is because the regionsInTransition map is mutated at bunch of places in the AssignmentManager class; so either I should invoke the method to compute and push the stats at all such callstacks, or may be we can have a similar approach as used by RegionServer.

        Show
        Himanshu Vashishtha added a comment - I have done some analysis and some questions regarding the metrics here. Please correct me if I am wrong. RegionServer updates its metrics at fix intervals defined by "hbase.regionserver.msginterval" and the metrics are pushed to the record after specified interval. Master don't use this methodology; rather whenever the master statistics are changed, the caller is supposed to update the metrics value on its own. I am wondering why we don't follow the same approach for master (because the stats we are recording are not changing that often?). The reason I am asking is because the regionsInTransition map is mutated at bunch of places in the AssignmentManager class; so either I should invoke the method to compute and push the stats at all such callstacks, or may be we can have a similar approach as used by RegionServer.
        Hide
        Himanshu Vashishtha added a comment -

        I have some some analysis and some questions regarding the metrics here. Please correct me if I am wrong.

        RegionServer updates its metrics at fix intervals defined by "hbase.regionserver.msginterval" and the metrics are pushed to the record after specified interval.
        Master don't use this methodology; rather whenever the master statistics are changed, the caller is supposed to update the metrics value on its own.

        I am wondering why we don't follow the same approach for master (because the stats we are recording are not changing that often?). The reason I am asking is because the regionsInTransition map is mutated at bunch of places in the AssignmentManager class; so either I should invoke the method to compute and push the stats at all such callstacks, or may be we can have a similar approach as used by RegionServer.

        Show
        Himanshu Vashishtha added a comment - I have some some analysis and some questions regarding the metrics here. Please correct me if I am wrong. RegionServer updates its metrics at fix intervals defined by "hbase.regionserver.msginterval" and the metrics are pushed to the record after specified interval. Master don't use this methodology; rather whenever the master statistics are changed, the caller is supposed to update the metrics value on its own. I am wondering why we don't follow the same approach for master (because the stats we are recording are not changing that often?). The reason I am asking is because the regionsInTransition map is mutated at bunch of places in the AssignmentManager class; so either I should invoke the method to compute and push the stats at all such callstacks, or may be we can have a similar approach as used by RegionServer.
        Hide
        Himanshu Vashishtha added a comment -

        yeah, I am working on the metrics. I am new to this part, so its taking a bit longer.

        Show
        Himanshu Vashishtha added a comment - yeah, I am working on the metrics. I am new to this part, so its taking a bit longer.
        Hide
        stack added a comment -

        @Himanshu Yeah, a line on the end w/ count of regions > one minute up in RIT would be good enough. You could make them yellow in the listing. But yeah, would be great if they came out as metrics as per Mr. Todd.

        Show
        stack added a comment - @Himanshu Yeah, a line on the end w/ count of regions > one minute up in RIT would be good enough. You could make them yellow in the listing. But yeah, would be great if they came out as metrics as per Mr. Todd.
        Hide
        Todd Lipcon added a comment -

        This patch should actually add metrics, not just UI entries. Adding to the UI is nice sugar, but exposing via JMX and the hadoop metrics interface is higher priority IMO.

        Show
        Todd Lipcon added a comment - This patch should actually add metrics , not just UI entries. Adding to the UI is nice sugar, but exposing via JMX and the hadoop metrics interface is higher priority IMO.
        Hide
        Himanshu Vashishtha added a comment -

        I like the idea of having only one table for RITs and adding a new column (at the end) showing in-transition time and marking the longest one in red.
        But the jira description also says to show number of regions that are in transition for more than a minute too? Should that be in a separate line below the table?

        (Sorry, my ui senses are quite raw I know)

        Show
        Himanshu Vashishtha added a comment - I like the idea of having only one table for RITs and adding a new column (at the end) showing in-transition time and marking the longest one in red. But the jira description also says to show number of regions that are in transition for more than a minute too? Should that be in a separate line below the table? (Sorry, my ui senses are quite raw I know)
        Hide
        stack added a comment -

        Can you make it so the table doesn't bump up against the first one?

        Should it be a separate table? Why not add sum on end and columns to the first table showing time in transition? Flag red the one that has been in transition the longest?

        Show
        stack added a comment - Can you make it so the table doesn't bump up against the first one? Should it be a separate table? Why not add sum on end and columns to the first table showing time in transition? Flag red the one that has been in transition the longest?
        Hide
        Himanshu Vashishtha added a comment -

        a sample rit snapshot

        Show
        Himanshu Vashishtha added a comment - a sample rit snapshot
        Hide
        Himanshu Vashishtha added a comment -

        added the time unit for Region in Transition' with maximum time

        Show
        Himanshu Vashishtha added a comment - added the time unit for Region in Transition' with maximum time
        Hide
        stack added a comment -

        @Himanshu That would be useful

        Show
        stack added a comment - @Himanshu That would be useful
        Hide
        Himanshu Vashishtha added a comment -

        It involves a UI related change, so I did test it on a 2 node cluster. Shall I attach a screenshot for the new metrics?

        Show
        Himanshu Vashishtha added a comment - It involves a UI related change, so I did test it on a 2 node cluster. Shall I attach a screenshot for the new metrics?
        Hide
        stack added a comment -

        See TestAssignmentManager. See how it can stand up a standalone AM and how then it fakes it out by manually adding stuff to zk (RITs, etc.)

        Show
        stack added a comment - See TestAssignmentManager. See how it can stand up a standalone AM and how then it fakes it out by manually adding stuff to zk (RITs, etc.)
        Hide
        Himanshu Vashishtha added a comment -

        I have created a patch, which involves a new method in org.apache.hadoop.hbase.master.AssignmentManager and supporting code in src/main/jamon/org/apache/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon.

        I am running it on my local system, and wonder about how to test this, i.e., to get some regions in RIT. Any suggestions please?

        Show
        Himanshu Vashishtha added a comment - I have created a patch, which involves a new method in org.apache.hadoop.hbase.master.AssignmentManager and supporting code in src/main/jamon/org/apache/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon. I am running it on my local system, and wonder about how to test this, i.e., to get some regions in RIT. Any suggestions please?

          People

          • Assignee:
            Himanshu Vashishtha
            Reporter:
            Todd Lipcon
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development