HBase
  1. HBase
  2. HBASE-5072

Support Max Value for Per-Store Metrics

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: metrics, regionserver
    • Labels:
      None

      Description

      We were bit in our multi-tenant cluster because one of our Stores encountered a bug and grew its StoreFile count. We didn't notice this because the StoreFile count currently reported by the RegionServer is an average of all Stores in the region. For the per-Store metrics, we should also record the max so we can notice outliers.

      1. ASF.LICENSE.NOT.GRANTED--D945.1.patch
        3 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--D945.2.patch
        3 kB
        Phabricator
      3. HBASE-5072.patch
        4 kB
        Nicolas Spiegelberg

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #39 (See https://builds.apache.org/job/HBase-TRUNK-security/39/)
        [jira] HBASE-5072 Support Max Value for Per-Store Metrics

        Summary:
        We were bit in our multi-tenant cluster because one of our Stores
        encountered a bug and grew its StoreFile count. We didn't notice this because
        the StoreFile count currently reported by the RegionServer is an average of all
        Stores in the region. For the per-Store metrics, we should also record the max
        so we can notice outliers.

        Test Plan: - mvn test -Dtest=TestRegionServerMetrics

        Reviewers: JIRA, mbautin, Kannan

        Reviewed By: Kannan

        CC: stack, nspiegelberg, mbautin, Kannan

        Differential Revision: 945

        nspiegelberg :
        Files :

        • /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 #39 (See https://builds.apache.org/job/HBase-TRUNK-security/39/ ) [jira] HBASE-5072 Support Max Value for Per-Store Metrics Summary: We were bit in our multi-tenant cluster because one of our Stores encountered a bug and grew its StoreFile count. We didn't notice this because the StoreFile count currently reported by the RegionServer is an average of all Stores in the region. For the per-Store metrics, we should also record the max so we can notice outliers. Test Plan: - mvn test -Dtest=TestRegionServerMetrics Reviewers: JIRA, mbautin, Kannan Reviewed By: Kannan CC: stack, nspiegelberg, mbautin, Kannan Differential Revision: 945 nspiegelberg : Files : /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
        Hudson added a comment -

        Integrated in HBase-TRUNK #2563 (See https://builds.apache.org/job/HBase-TRUNK/2563/)
        [jira] HBASE-5072 Support Max Value for Per-Store Metrics

        Summary:
        We were bit in our multi-tenant cluster because one of our Stores
        encountered a bug and grew its StoreFile count. We didn't notice this because
        the StoreFile count currently reported by the RegionServer is an average of all
        Stores in the region. For the per-Store metrics, we should also record the max
        so we can notice outliers.

        Test Plan: - mvn test -Dtest=TestRegionServerMetrics

        Reviewers: JIRA, mbautin, Kannan

        Reviewed By: Kannan

        CC: stack, nspiegelberg, mbautin, Kannan

        Differential Revision: 945

        nspiegelberg :
        Files :

        • /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 #2563 (See https://builds.apache.org/job/HBase-TRUNK/2563/ ) [jira] HBASE-5072 Support Max Value for Per-Store Metrics Summary: We were bit in our multi-tenant cluster because one of our Stores encountered a bug and grew its StoreFile count. We didn't notice this because the StoreFile count currently reported by the RegionServer is an average of all Stores in the region. For the per-Store metrics, we should also record the max so we can notice outliers. Test Plan: - mvn test -Dtest=TestRegionServerMetrics Reviewers: JIRA, mbautin, Kannan Reviewed By: Kannan CC: stack, nspiegelberg, mbautin, Kannan Differential Revision: 945 nspiegelberg : Files : /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
        Phabricator added a comment -

        nspiegelberg has committed the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        REVISION DETAIL
        https://reviews.facebook.net/D945

        COMMIT
        https://reviews.facebook.net/rHBASE1221419

        Show
        Phabricator added a comment - nspiegelberg has committed the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". REVISION DETAIL https://reviews.facebook.net/D945 COMMIT https://reviews.facebook.net/rHBASE1221419
        Hide
        Nicolas Spiegelberg added a comment -

        note: patch applies cleanly to both 89-fb & trunk

        Show
        Nicolas Spiegelberg added a comment - note: patch applies cleanly to both 89-fb & trunk
        Hide
        Phabricator added a comment -

        Kannan has commented on the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        s/go it/got it.

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". s/go it/got it. REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        Kannan has commented on the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        Ok – go it. You are tracking the max only across all CFs. Sounds good. Thanks for the clarification. I misread the code there.

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". Ok – go it. You are tracking the max only across all CFs. Sounds good. Thanks for the clarification. I misread the code there. REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        nspiegelberg has commented on the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:388 actually, this comment is correct. Note that 'ALL_SCHEMA_METRICS' is a well-known singleton to store the per-server aggregation of these metrics.

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - nspiegelberg has commented on the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:388 actually, this comment is correct. Note that 'ALL_SCHEMA_METRICS' is a well-known singleton to store the per-server aggregation of these metrics. REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        Kannan has accepted the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        +1.

        One small inlined comment.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:388 just noticed a minor thing in the comment.

        Perhaps this comment:

        also compute the max value across all Stores on this server

        should read:

        also compute the max value for a given Store (CF) across all regions on this server.

        ?

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - Kannan has accepted the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". +1. One small inlined comment. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:388 just noticed a minor thing in the comment. Perhaps this comment: also compute the max value across all Stores on this server should read: also compute the max value for a given Store (CF) across all regions on this server. ? REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        nspiegelberg updated the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".
        Reviewers: JIRA, mbautin, Kannan

        Added Kannan's peer review optimization

        REVISION DETAIL
        https://reviews.facebook.net/D945

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java

        Show
        Phabricator added a comment - nspiegelberg updated the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". Reviewers: JIRA, mbautin, Kannan Added Kannan's peer review optimization REVISION DETAIL https://reviews.facebook.net/D945 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
        Hide
        Phabricator added a comment -

        Kannan has commented on the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:392 minor: Since this is already a map with MutableDoubles, you can break this into two cases to avoid new allocations when possible. Something like:

        if (cur == null)

        { tmpMap.put(maxKey, new MutableDouble(val); }

        else if (cur.doubleValue() < val)

        { cur.setValue(val); }

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:392 minor: Since this is already a map with MutableDoubles, you can break this into two cases to avoid new allocations when possible. Something like: if (cur == null) { tmpMap.put(maxKey, new MutableDouble(val); } else if (cur.doubleValue() < val) { cur.setValue(val); } REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        nspiegelberg has commented on the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".
        Added CCs: mbautin

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:387 note that we're doing a pointer comparison here. The 'this' pointer is being compared to a well-known singleton. This patter is used throughout the class. I switched the logic to equals because too many negations leads to confusion.
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:234 correct. I talked to @mbautin about this today. The idea is to reduce the number of heap allocations when doing metrics. I don't think this is as big an issue, since heap allocations on the par-new space should be really efficient, but I'm keeping with the existing style

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - nspiegelberg has commented on the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". Added CCs: mbautin INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:387 note that we're doing a pointer comparison here. The 'this' pointer is being compared to a well-known singleton. This patter is used throughout the class. I switched the logic to equals because too many negations leads to confusion. src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:234 correct. I talked to @mbautin about this today. The idea is to reduce the number of heap allocations when doing metrics. I don't think this is as big an issue, since heap allocations on the par-new space should be really efficient, but I'm keeping with the existing style REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        stack has commented on the revision "[jira] HBASE-5072 Support Max Value for Per-Store Metrics".

        LGTM

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:234 This array is meant to be NUM_STORE_METRIC_TYPES?
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:387 This is not your change but am I reading this right? 'this' == to a DEFINE? The 'this' is object's toString? I suppose it makes sense but tad confusing.

        REVISION DETAIL
        https://reviews.facebook.net/D945

        Show
        Phabricator added a comment - stack has commented on the revision " [jira] HBASE-5072 Support Max Value for Per-Store Metrics". LGTM INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:234 This array is meant to be NUM_STORE_METRIC_TYPES? src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:387 This is not your change but am I reading this right? 'this' == to a DEFINE? The 'this' is object's toString? I suppose it makes sense but tad confusing. REVISION DETAIL https://reviews.facebook.net/D945
        Hide
        Phabricator added a comment -

        nspiegelberg requested code review of "[jira] HBASE-5072".
        Reviewers: JIRA, mbautin, Kannan

        TEST PLAN

        • mvn test -Dtest=TestRegionServerMetrics

        REVISION DETAIL
        https://reviews.facebook.net/D945

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
        src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/1953/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - nspiegelberg requested code review of " [jira] HBASE-5072 ". Reviewers: JIRA, mbautin, Kannan TEST PLAN mvn test -Dtest=TestRegionServerMetrics REVISION DETAIL https://reviews.facebook.net/D945 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/1953/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.

          People

          • Assignee:
            Nicolas Spiegelberg
            Reporter:
            Nicolas Spiegelberg
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development