HBase
  1. HBase
  2. HBASE-5788

Move Dynamic Metrics storage off of HRegion.

    Details

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

      Description

      HRegion right now has the responsibility of storing static counts and latency numbers for use by the metrics package. Since these maps are incremented and set from lots of places it makes adding functionality hard.

      So move the metrics functionality into SchemaMetrics making it more than just a class for naming. The next step will be to simplify the api exposed so that using it will be easier.

      1. HBASE-5788-4.patch
        27 kB
        Elliott Clark
      2. HBASE-5788-3.patch
        26 kB
        Elliott Clark
      3. HBASE-5788-2.patch
        26 kB
        Elliott Clark
      4. HBASE-5788-1.patch
        25 kB
        Elliott Clark
      5. HBASE-5788-0.patch
        23 kB
        Elliott Clark

        Issue Links

          Activity

          Hide
          Elliott Clark added a comment -

          On trying to add more to this class it became obvious to me that it needs more love than the first try. I'll roll this into exposing per region metrics.

          Show
          Elliott Clark added a comment - On trying to add more to this class it became obvious to me that it needs more love than the first try. I'll roll this into exposing per region metrics.
          Hide
          Elliott Clark added a comment -

          Better version. SchemaMetrics wasn't the right place to put it. While it does a lot of the interaction there are lots of other places so a static MetricsStorage seems better.

          Show
          Elliott Clark added a comment - Better version. SchemaMetrics wasn't the right place to put it. While it does a lot of the interaction there are lots of other places so a static MetricsStorage seems better.
          Hide
          Elliott Clark added a comment -

          Added header to new file.

          Show
          Elliott Clark added a comment - Added header to new file.
          Hide
          stack added a comment -

          Is MetricsStorage for Region metrics only? If so, call it RegionMetrics? Or maybe its generic metrics storage for this package? If so, the name is right. Should it be down in the metrics package? Regardless, new class needs class comment explaining class scope. Does it have to public? Can it be private to the package at least? Lines < 100 chars. Why are data members in this new class public rather than private? Even if they are static. And static data members probably ain't a good idea because then there is one only per JVM and there can be many regionservers in the one JVM; e.g. in testing. Yeah, do its method names need to be public? Can these be package private? Hmm... maybe they need to be public because called from the metrics subpackage? I like all the code that comes out of HRegion. Thats good. And no harm in a basic unit test that your new class is basically working. Any worries w/ concurrent access? Good stuff Elliott.

          Show
          stack added a comment - Is MetricsStorage for Region metrics only? If so, call it RegionMetrics? Or maybe its generic metrics storage for this package? If so, the name is right. Should it be down in the metrics package? Regardless, new class needs class comment explaining class scope. Does it have to public? Can it be private to the package at least? Lines < 100 chars. Why are data members in this new class public rather than private? Even if they are static. And static data members probably ain't a good idea because then there is one only per JVM and there can be many regionservers in the one JVM; e.g. in testing. Yeah, do its method names need to be public? Can these be package private? Hmm... maybe they need to be public because called from the metrics subpackage? I like all the code that comes out of HRegion. Thats good. And no harm in a basic unit test that your new class is basically working. Any worries w/ concurrent access? Good stuff Elliott.
          Hide
          Elliott Clark added a comment -

          RegionMetricsStorage could be a better name this class holds and mutates the numbers that will be exposed as dynamic region metrics.

          I'll add the comments, and check the line length. Not sure how I messed up eclipse.

          The data members can be package private now that they are in the regionserver.metrics package. I'll check to make sure.

          Everything else is accessed in the regionserver package so they need to be public :-/

          TestRegionServerMetrics covers most of the functionality of the new class but I can create a new set of more explicit tests if you think that is needed.

          Show
          Elliott Clark added a comment - RegionMetricsStorage could be a better name this class holds and mutates the numbers that will be exposed as dynamic region metrics. I'll add the comments, and check the line length. Not sure how I messed up eclipse. The data members can be package private now that they are in the regionserver.metrics package. I'll check to make sure. Everything else is accessed in the regionserver package so they need to be public :-/ TestRegionServerMetrics covers most of the functionality of the new class but I can create a new set of more explicit tests if you think that is needed.
          Hide
          stack added a comment -

          TestRegionServerMetrics covers most of the functionality of the new class but I can create a new set of more explicit tests if you think that is needed.

          Probably no need if we have some coverage already. Just want to make sure the class does its basic contract. Easier figuring this stuff in a unit test than up on a cluster, yadda, yadda, you know what I'm at.

          Show
          stack added a comment - TestRegionServerMetrics covers most of the functionality of the new class but I can create a new set of more explicit tests if you think that is needed. Probably no need if we have some coverage already. Just want to make sure the class does its basic contract. Easier figuring this stuff in a unit test than up on a cluster, yadda, yadda, you know what I'm at.
          Hide
          Elliott Clark added a comment -

          Lines should all be < 100.

          Added comments on RegionMetricsStorage.

          Made data in RegionMetricsStorage private with public getters (tests require it to be public).

          Show
          Elliott Clark added a comment - Lines should all be < 100. Added comments on RegionMetricsStorage. Made data in RegionMetricsStorage private with public getters (tests require it to be public).
          Hide
          Elliott Clark added a comment -

          Add @InterfaceAudience.Private to the new class.

          Show
          Elliott Clark added a comment - Add @InterfaceAudience.Private to the new class.
          Hide
          stack added a comment -

          lgtm

          Trying against hadoopqa

          Show
          stack added a comment - lgtm Trying against hadoopqa
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12523008/HBASE-5788-4.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 appears to introduce 4 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.replication.TestReplication

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1557//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1557//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1557//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/12523008/HBASE-5788-4.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 appears to introduce 4 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.replication.TestReplication Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1557//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1557//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1557//console This message is automatically generated.
          Hide
          stack added a comment -

          Applied to Trunk (Just TestReplication failed... which seems unrelated). Thanks for the patch Elliott.

          Show
          stack added a comment - Applied to Trunk (Just TestReplication failed... which seems unrelated). Thanks for the patch Elliott.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2776 (See https://builds.apache.org/job/HBase-TRUNK/2776/)
          HBASE-5788 Move Dynamic Metrics storage off of HRegion (Revision 1327316)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionMetricsStorage.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionServerDynamicMetrics.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2776 (See https://builds.apache.org/job/HBase-TRUNK/2776/ ) HBASE-5788 Move Dynamic Metrics storage off of HRegion (Revision 1327316) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionMetricsStorage.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionServerDynamicMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
          Hide
          Enis Soztutar added a comment -

          The next step will be to simplify the api exposed so that using it will be easier.

          @Elliot, do you have something in mind for this? While working on HBASE-5601, I also noticed that SchemaMetrics needs some simplification as well.

          Show
          Enis Soztutar added a comment - The next step will be to simplify the api exposed so that using it will be easier. @Elliot, do you have something in mind for this? While working on HBASE-5601 , I also noticed that SchemaMetrics needs some simplification as well.
          Hide
          Elliott Clark added a comment -

          @Enis I was thinking of getting things more like HBASE-3614.

          I moved metrics about get/put/delete/icv into a helper class that mutates data in RegionMetricsStorage. I was hoping to go that way with pretty simple helper classes that expose update methods. Then SchemaMetrics can be mutated in the same way and metric naming can be handled by static helpers only.

          Show
          Elliott Clark added a comment - @Enis I was thinking of getting things more like HBASE-3614 . I moved metrics about get/put/delete/icv into a helper class that mutates data in RegionMetricsStorage. I was hoping to go that way with pretty simple helper classes that expose update methods. Then SchemaMetrics can be mutated in the same way and metric naming can be handled by static helpers only.
          Hide
          Enis Soztutar added a comment -

          I moved metrics about get/put/delete/icv into a helper class that mutates data in RegionMetricsStorage. I was hoping to go that way with pretty simple helper classes that expose update methods. Then SchemaMetrics can be mutated in the same way and metric naming can be handled by static helpers only.

          Ok, i see. Helper methods should help. But I think we should get rid of the static state there.

          I also left a comment on HBASE-3614.

          Show
          Enis Soztutar added a comment - I moved metrics about get/put/delete/icv into a helper class that mutates data in RegionMetricsStorage. I was hoping to go that way with pretty simple helper classes that expose update methods. Then SchemaMetrics can be mutated in the same way and metric naming can be handled by static helpers only. Ok, i see. Helper methods should help. But I think we should get rid of the static state there. I also left a comment on HBASE-3614 .
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #174 (See https://builds.apache.org/job/HBase-TRUNK-security/174/)
          HBASE-5788 Move Dynamic Metrics storage off of HRegion (Revision 1327316)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionMetricsStorage.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionServerDynamicMetrics.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #174 (See https://builds.apache.org/job/HBase-TRUNK-security/174/ ) HBASE-5788 Move Dynamic Metrics storage off of HRegion (Revision 1327316) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionMetricsStorage.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/RegionServerDynamicMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java

            People

            • Assignee:
              Elliott Clark
              Reporter:
              Elliott Clark
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development