HBase
  1. HBase
  2. HBASE-5744

Thrift server metrics should be long instead of int

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere.

        Activity

        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int".
        Reviewers: sc, dhruba, Kannan, Liyin, JIRA

        As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process.

        TEST PLAN
        TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix.

        Re-run all unit tests.

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

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

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

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

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int". Reviewers: sc, dhruba, Kannan, Liyin, JIRA As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process. TEST PLAN TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix. Re-run all unit tests. REVISION DETAIL https://reviews.facebook.net/D2679 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/6135/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        sc has accepted the revision "[jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int".

        Looks good to me. Thanks a lot for the fix!

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

        BRANCH
        thrift_server_metrics_should_be_long_HBASE-5744_v2

        Show
        Phabricator added a comment - sc has accepted the revision " [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int". Looks good to me. Thanks a lot for the fix! REVISION DETAIL https://reviews.facebook.net/D2679 BRANCH thrift_server_metrics_should_be_long_ HBASE-5744 _v2
        Hide
        Phabricator added a comment -

        stack has commented on the revision "[jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int".

        lgtm. Do we need this on trunk too MIkhail?

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

        BRANCH
        thrift_server_metrics_should_be_long_HBASE-5744_v2

        Show
        Phabricator added a comment - stack has commented on the revision " [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int". lgtm. Do we need this on trunk too MIkhail? REVISION DETAIL https://reviews.facebook.net/D2679 BRANCH thrift_server_metrics_should_be_long_ HBASE-5744 _v2
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int".

        Scott: thanks for reviewing!

        Stack: yes, I will post a trunk patch in a bit.

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

        BRANCH
        thrift_server_metrics_should_be_long_HBASE-5744_v2

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int". Scott: thanks for reviewing! Stack: yes, I will post a trunk patch in a bit. REVISION DETAIL https://reviews.facebook.net/D2679 BRANCH thrift_server_metrics_should_be_long_ HBASE-5744 _v2
        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HBASE-5744 Thrift server metrics should be long instead of int".
        Reviewers: stack, sc, Kannan, JIRA

        As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process.

        This is a trunk diff. The 89-fb version of this diff is at D2679.

        TEST PLAN
        TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix.

        Re-run all unit tests.

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

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

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

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

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5744 Thrift server metrics should be long instead of int". Reviewers: stack, sc, Kannan, JIRA As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process. This is a trunk diff. The 89-fb version of this diff is at D2679. TEST PLAN TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix. Re-run all unit tests. REVISION DETAIL https://reviews.facebook.net/D2685 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/6153/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        sc has accepted the revision "[jira] HBASE-5744 Thrift server metrics should be long instead of int".

        +1 Thanks for fixing the trunk!

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

        BRANCH
        thrift_server_metrics_should_be_long_HBASE-5744_v2

        Show
        Phabricator added a comment - sc has accepted the revision " [jira] HBASE-5744 Thrift server metrics should be long instead of int". +1 Thanks for fixing the trunk! REVISION DETAIL https://reviews.facebook.net/D2685 BRANCH thrift_server_metrics_should_be_long_ HBASE-5744 _v2
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521872/D2685.1.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 passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1446//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1446//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1446//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/12521872/D2685.1.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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1446//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1446//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1446//console This message is automatically generated.
        Hide
        Mikhail Bautin added a comment -

        The same patch (re-attaching to run a test on Jenkins).

        Show
        Mikhail Bautin added a comment - The same patch (re-attaching to run a test on Jenkins).
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521874/jira-HBASE-5744-89-fb-Thrift-server-metrics-should-b-2012-04-07_21_39_35.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 5 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 passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1447//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1447//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1447//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/12521874/jira-HBASE-5744-89-fb-Thrift-server-metrics-should-b-2012-04-07_21_39_35.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1447//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1447//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1447//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        mbautin has committed the revision "[jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int".

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

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

        Show
        Phabricator added a comment - mbautin has committed the revision " [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int". REVISION DETAIL https://reviews.facebook.net/D2679 COMMIT https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1310973
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5744 Thrift server metrics should be long instead of int".
        Reviewers: stack, sc, Kannan, JIRA

        Removing the old unused overload of verifyMetrics.

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

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5744 Thrift server metrics should be long instead of int". Reviewers: stack, sc, Kannan, JIRA Removing the old unused overload of verifyMetrics. REVISION DETAIL https://reviews.facebook.net/D2685 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5744 Thrift server metrics should be long instead of int".
        Reviewers: stack, sc, Kannan, JIRA

        Submitted incomplete diff last time, adding other changes.

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5744 Thrift server metrics should be long instead of int". Reviewers: stack, sc, Kannan, JIRA Submitted incomplete diff last time, adding other changes. REVISION DETAIL https://reviews.facebook.net/D2685 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12521924/D2685.3.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/1452//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/12521924/D2685.3.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/1452//console This message is automatically generated.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2729 (See https://builds.apache.org/job/HBase-TRUNK/2729/)
        [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int

        Summary: As we measure our Thrift call latencies in nanoseconds, we need to make
        latencies long instead of int everywhere. There is a bug where we truncate a
        nanosecond latency to int, which is a problem with RPCs that take more than
        2.147483647 seconds to process.

        Test Plan:
        TestThriftServer is updated to test for the failure case (an RPC is artificially
        made to take 3 seconds). The new test case fails without the fix.

        Re-run all unit tests.

        Reviewers: sc, dhruba, Kannan, Liyin, JIRA

        Reviewed By: sc

        CC: stack

        Differential Revision: https://reviews.facebook.net/D2679 (Revision 1311167)

        Result = SUCCESS
        mbautin :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2729 (See https://builds.apache.org/job/HBase-TRUNK/2729/ ) [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int Summary: As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process. Test Plan: TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix. Re-run all unit tests. Reviewers: sc, dhruba, Kannan, Liyin, JIRA Reviewed By: sc CC: stack Differential Revision: https://reviews.facebook.net/D2679 (Revision 1311167) Result = SUCCESS mbautin : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #164 (See https://builds.apache.org/job/HBase-TRUNK-security/164/)
        [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int

        Summary: As we measure our Thrift call latencies in nanoseconds, we need to make
        latencies long instead of int everywhere. There is a bug where we truncate a
        nanosecond latency to int, which is a problem with RPCs that take more than
        2.147483647 seconds to process.

        Test Plan:
        TestThriftServer is updated to test for the failure case (an RPC is artificially
        made to take 3 seconds). The new test case fails without the fix.

        Re-run all unit tests.

        Reviewers: sc, dhruba, Kannan, Liyin, JIRA

        Reviewed By: sc

        CC: stack

        Differential Revision: https://reviews.facebook.net/D2679 (Revision 1311167)

        Result = FAILURE
        mbautin :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #164 (See https://builds.apache.org/job/HBase-TRUNK-security/164/ ) [jira] HBASE-5744 [89-fb] Thrift server metrics should be long instead of int Summary: As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process. Test Plan: TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix. Re-run all unit tests. Reviewers: sc, dhruba, Kannan, Liyin, JIRA Reviewed By: sc CC: stack Differential Revision: https://reviews.facebook.net/D2679 (Revision 1311167) Result = FAILURE mbautin : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Hide
        Phabricator added a comment -

        mbautin has abandoned the revision "[jira] HBASE-5744 Thrift server metrics should be long instead of int".

        Unfortunately, I committed this diff under the 89-fb differential revision first (D2679) and had to edit the commit message. Here is what I did:

        > svn propedit -r 1311167 --revprop svn:log

        The new commit message is as follows:

        > svn log -r1311167 https://svn.apache.org/repos/asf/hbase/trunk
        ------------------------------------------------------------------------
        r1311167 | mbautin | 2012-04-09 02:00:11 -0700 (Mon, 09 Apr 2012) | 23 lines

        [jira] HBASE-5744 Thrift server metrics should be long instead of int

        Summary:
        As we measure our Thrift call latencies in nanoseconds, we need to make
        latencies long instead of int everywhere. There is a bug where we truncate a
        nanosecond latency to int, which is a problem with RPCs that take more than
        2.147483647 seconds to process.

        This is a trunk diff. The 89-fb version of this diff is at D2679.

        Test Plan:
        TestThriftServer is updated to test for the failure case (an RPC is artificially
        made to take 3 seconds). The new test case fails without the fix.

        Re-run all unit tests.

        Reviewers: stack, sc, Kannan, JIRA

        Reviewed By: sc

        Differential Revision: https://reviews.facebook.net/D2685

        If you have a git-svn checkout that got confused by this, here is how to get it back to a consistent state:

        git svn reset 1311166
        git svn rebase

        Sorry about the inconvenience.

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

        Show
        Phabricator added a comment - mbautin has abandoned the revision " [jira] HBASE-5744 Thrift server metrics should be long instead of int". Unfortunately, I committed this diff under the 89-fb differential revision first (D2679) and had to edit the commit message. Here is what I did: > svn propedit -r 1311167 --revprop svn:log The new commit message is as follows: > svn log -r1311167 https://svn.apache.org/repos/asf/hbase/trunk ------------------------------------------------------------------------ r1311167 | mbautin | 2012-04-09 02:00:11 -0700 (Mon, 09 Apr 2012) | 23 lines [jira] HBASE-5744 Thrift server metrics should be long instead of int Summary: As we measure our Thrift call latencies in nanoseconds, we need to make latencies long instead of int everywhere. There is a bug where we truncate a nanosecond latency to int, which is a problem with RPCs that take more than 2.147483647 seconds to process. This is a trunk diff. The 89-fb version of this diff is at D2679. Test Plan: TestThriftServer is updated to test for the failure case (an RPC is artificially made to take 3 seconds). The new test case fails without the fix. Re-run all unit tests. Reviewers: stack, sc, Kannan, JIRA Reviewed By: sc Differential Revision: https://reviews.facebook.net/D2685 If you have a git-svn checkout that got confused by this, here is how to get it back to a consistent state: git svn reset 1311166 git svn rebase Sorry about the inconvenience. REVISION DETAIL https://reviews.facebook.net/D2685
        Hide
        Mikhail Bautin added a comment -

        Committed to trunk.

        Show
        Mikhail Bautin added a comment - Committed to trunk.

          People

          • Assignee:
            Mikhail Bautin
            Reporter:
            Mikhail Bautin
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development