Details

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

      Description

      It will be useful to have some metrics (queue length, waiting time, processing time ...) similar to Hadoop RPC server. This allows us to monitor system health also provide a tool to diagnose the problem where thrift calls are slow.

      1. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.8.patch
        34 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.7.patch
        34 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.6.patch
        33 kB
        Phabricator
      4. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.5.patch
        32 kB
        Phabricator
      5. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.4.patch
        32 kB
        Phabricator
      6. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.3.patch
        32 kB
        Phabricator
      7. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.2.patch
        27 kB
        Phabricator
      8. ASF.LICENSE.NOT.GRANTED--HBASE-5186.D1461.1.patch
        28 kB
        Phabricator
      9. 5186-v9.txt
        34 kB
        Ted Yu
      10. 5186-v12.txt
        34 kB
        Ted Yu
      11. 5186-v11.txt
        36 kB
        Ted Yu
      12. 5186-v10.txt
        34 kB
        Ted Yu

        Activity

        Scott Chen created issue -
        Hide
        Phabricator added a comment -

        sc requested code review of "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA

        Add metrics to ThriftServer

        It will be useful to have some metrics (queue length, waiting time, processing
        time ...) similar to Hadoop RPC server. This allows us to monitor system health
        also provide a tool to diagnose the problem where thrift calls are slow.

        It will be useful to have some metrics (queue length, waiting time, processing time ...) similar to Hadoop RPC server. This allows us to monitor system health also provide a tool to diagnose the problem where thrift calls are slow.

        TEST PLAN
        EMPTY

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

        AFFECTED FILES
        pom.xml
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.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/3021/

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

        Show
        Phabricator added a comment - sc requested code review of " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA Add metrics to ThriftServer It will be useful to have some metrics (queue length, waiting time, processing time ...) similar to Hadoop RPC server. This allows us to monitor system health also provide a tool to diagnose the problem where thrift calls are slow. It will be useful to have some metrics (queue length, waiting time, processing time ...) similar to Hadoop RPC server. This allows us to monitor system health also provide a tool to diagnose the problem where thrift calls are slow. TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES pom.xml src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.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/3021/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Phabricator made changes -
        Field Original Value New Value
        Attachment HBASE-5186.D1461.1.patch [ 12511939 ]
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA

        Remove the debug change

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

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

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA Remove the debug change REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.2.patch [ 12511940 ]
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA

        Add TestCallQueue

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

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

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA Add TestCallQueue REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.3.patch [ 12511943 ]
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA, heyongqiang

        Remove some dead variables

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA, heyongqiang Remove some dead variables REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.4.patch [ 12511948 ]
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA, heyongqiang

        Remove unnecessary locking in ThriftMetrics

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA, heyongqiang Remove unnecessary locking in ThriftMetrics REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.5.patch [ 12512070 ]
        Scott Chen made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Scott Chen made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512070/HBASE-5186.D1461.5.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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 161 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.client.TestFromClientSide
        org.apache.hadoop.hbase.io.hfile.TestHFileBlock
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/865//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/865//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/865//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/12512070/HBASE-5186.D1461.5.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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 161 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.client.TestFromClientSide org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/865//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/865//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/865//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java:2 Year is not needed.
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java:44 Would ThriftMetrics be created for thrift2 ?
        Fine if you plan to do it in another JIRA.
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java:53 As the code on line 102 shows, "hbase.thrift.slow.response.nano.second" may be a better name for this config.
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:57 Would timeInQueue be a better method name ?
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:29 Does the call queue length in metrics match 'total elements removed' in javadoc ?
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:128 According to http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/BlockingQueue.html#drainTo%28java.util.Collection%29:

        Attempts to drain a queue to itself result in IllegalArgumentException.

        We should maintain the above semantics.
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:153 Can we add the new Call object to underlyingQueue ?
        This way we don't need to create toBeAdded

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java:2 Year is not needed. src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java:44 Would ThriftMetrics be created for thrift2 ? Fine if you plan to do it in another JIRA. src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java:53 As the code on line 102 shows, "hbase.thrift.slow.response.nano.second" may be a better name for this config. src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:57 Would timeInQueue be a better method name ? src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:29 Does the call queue length in metrics match 'total elements removed' in javadoc ? src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:128 According to http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/BlockingQueue.html#drainTo%28java.util.Collection%29: Attempts to drain a queue to itself result in IllegalArgumentException. We should maintain the above semantics. src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:153 Can we add the new Call object to underlyingQueue ? This way we don't need to create toBeAdded REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        Methods in class CallQueue look packed.
        Usually we leave one empty line between methods.

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Methods in class CallQueue look packed. Usually we leave one empty line between methods. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        Ted: Thanks for reviewing. I will make the change to this patch soon. I will make the change for thrift2 in another JIRA.

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Ted: Thanks for reviewing. I will make the change to this patch soon. I will make the change for thrift2 in another JIRA. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Scott Chen added a comment -

        I have opened HBASE-5298 for thrift2 metrics.

        Show
        Scott Chen added a comment - I have opened HBASE-5298 for thrift2 metrics.
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:128 good catch

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:128 good catch REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA, heyongqiang

        Addressed Ted's comments

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA, heyongqiang Addressed Ted's comments REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.6.patch [ 12512410 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512410/HBASE-5186.D1461.6.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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 161 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/871//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/871//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/871//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/12512410/HBASE-5186.D1461.6.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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 161 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/871//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/871//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/871//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:67 Should we consider startTime when performing equality check ?
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:77 Same comment as above: include startTime in hash code ?
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:34 The class is CallQueue. Should we tighten the type parameter to Call ?

        The reasoning is that users of CallQueue should use offer(), put(), etc to insert Runnable's and these methods wrap the Runnable in Call already.

        That way we can save instanceof checks.

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:67 Should we consider startTime when performing equality check ? src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:77 Same comment as above: include startTime in hash code ? src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:34 The class is CallQueue. Should we tighten the type parameter to Call ? The reasoning is that users of CallQueue should use offer(), put(), etc to insert Runnable's and these methods wrap the Runnable in Call already. That way we can save instanceof checks. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:34 good idea. I will make the change.

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:34 good idea. I will make the change. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA, heyongqiang

        Make underlyingQueue a BlockingQueue<Call>.

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA, heyongqiang Make underlyingQueue a BlockingQueue<Call>. REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.7.patch [ 12512562 ]
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:67 I am not sure if we should check startTime. One might want to remove a Runnable added to the CallQueue (which is a BlockingQueue<Runnable>). Adding the startTime check will remove this ability.

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:67 I am not sure if we should check startTime. One might want to remove a Runnable added to the CallQueue (which is a BlockingQueue<Runnable>). Adding the startTime check will remove this ability. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512562/HBASE-5186.D1461.7.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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 161 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.io.hfile.TestLruBlockCache
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/881//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/881//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/881//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/12512562/HBASE-5186.D1461.7.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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 161 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.io.hfile.TestLruBlockCache org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/881//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/881//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/881//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:126 We iterate underlyingQueue twice.
        Can we reduce to one iteration ? We can call underlyingQueue.clear() at the end.
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:125 Can we reuse the drainTo() at line 134 and specify Integer.MAX_VALUE here ?
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:141 Reduce to one iteration.
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java:34 Should read 'each call'

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:126 We iterate underlyingQueue twice. Can we reduce to one iteration ? We can call underlyingQueue.clear() at the end. src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:125 Can we reuse the drainTo() at line 134 and specify Integer.MAX_VALUE here ? src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:141 Reduce to one iteration. src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java:34 Should read 'each call' REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        @tedyu: Thanks! The comments make sense. I will update the patch soon.

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". @tedyu: Thanks! The comments make sense. I will update the patch soon. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:126 @tedyu: On second thought, I think using draintTo() might still be better.

        Iterating underlyingQueue and then calling clear() doesn't work for the case that maxElements is given. Also there may be some thread-safe issues need to be solved.

        Am I understanding your comment correctly? What do you think?

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:126 @tedyu: On second thought, I think using draintTo() might still be better. Iterating underlyingQueue and then calling clear() doesn't work for the case that maxElements is given. Also there may be some thread-safe issues need to be solved. Am I understanding your comment correctly? What do you think? REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc updated the revision "HBASE-5186 [jira] Add metrics to ThriftServer".
        Reviewers: dhruba, tedyu, JIRA, heyongqiang

        Addressed Ted's comments

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

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

        Show
        Phabricator added a comment - sc updated the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Reviewers: dhruba, tedyu, JIRA, heyongqiang Addressed Ted's comments REVISION DETAIL https://reviews.facebook.net/D1461 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Phabricator made changes -
        Attachment HBASE-5186.D1461.8.patch [ 12512748 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512748/HBASE-5186.D1461.8.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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 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.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/887//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/887//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/887//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/12512748/HBASE-5186.D1461.8.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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 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.coprocessor.TestMasterObserver org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/887//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/887//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/887//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        Thanks for the hard work.
        One last comment

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Good point about clear() unsuitable for this method.

        Can we use underlyingQueue.iterator() and remove at most maxElements elements from underlyingQueue ?

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". Thanks for the hard work. One last comment INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Good point about clear() unsuitable for this method. Can we use underlyingQueue.iterator() and remove at most maxElements elements from underlyingQueue ? REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Ted Yu added a comment -

        Patch v9 is almost same as patch v8.
        I used an iterator to traverse underlyingQueue in drainTo().

        @Scott:
        If v9 looks good to you, I will integrate tomorrow.

        Show
        Ted Yu added a comment - Patch v9 is almost same as patch v8. I used an iterator to traverse underlyingQueue in drainTo(). @Scott: If v9 looks good to you, I will integrate tomorrow.
        Ted Yu made changes -
        Attachment 5186-v9.txt [ 12512780 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512780/5186-v9.txt
        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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 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.client.TestAdmin
        org.apache.hadoop.hbase.replication.TestReplicationPeer
        org.apache.hadoop.hbase.io.hfile.TestHFileBlock
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/888//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/888//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/888//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/12512780/5186-v9.txt 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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 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.client.TestAdmin org.apache.hadoop.hbase.replication.TestReplicationPeer org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/888//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/888//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/888//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Ran the two additional failed tests locally based on patch v9 and they passed:

          822  mt -Dtest=TestReplicationPeer
          824  mt -Dtest=TestAdmin
        
        Show
        Ted Yu added a comment - Ran the two additional failed tests locally based on patch v9 and they passed: 822 mt -Dtest=TestReplicationPeer 824 mt -Dtest=TestAdmin
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 If there is another thread also adding elements at the same time, we may remove the wrong element. It is not clean to me how to solve the concurrency problem here.

        Ted: Do you have any suggestions?

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 If there is another thread also adding elements at the same time, we may remove the wrong element. It is not clean to me how to solve the concurrency problem here. Ted: Do you have any suggestions? REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Good point.
        Addressed the concurrency issue in patch v10.

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Good point. Addressed the concurrency issue in patch v10. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Ted Yu added a comment -

        Patch v10 addresses concurrency issue in drainTo().

        Show
        Ted Yu added a comment - Patch v10 addresses concurrency issue in drainTo().
        Ted Yu made changes -
        Attachment 5186-v10.txt [ 12512873 ]
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Ted: Looked at v10. It seems to me that we also need to lock this.lock while invoking other methods (take, poll...) on underlyingQueue. Otherwise the lock may not be protecting these operations.

        But if we do that the performance may be degraded because some of the BlockingQueue use only compareAndSet with no locking.

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Ted: Looked at v10. It seems to me that we also need to lock this.lock while invoking other methods (take, poll...) on underlyingQueue. Otherwise the lock may not be protecting these operations. But if we do that the performance may be degraded because some of the BlockingQueue use only compareAndSet with no locking. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 I don't see compareAndSet in http://fuseyism.com/classpath/doc/java/util/concurrent/ArrayBlockingQueue-source.html
        Or here: http://fuseyism.com/classpath/doc/java/util/concurrent/LinkedBlockingQueue-source.html

        Will upload patch v11 which adds protection for other methods.

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 I don't see compareAndSet in http://fuseyism.com/classpath/doc/java/util/concurrent/ArrayBlockingQueue-source.html Or here: http://fuseyism.com/classpath/doc/java/util/concurrent/LinkedBlockingQueue-source.html Will upload patch v11 which adds protection for other methods. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Ted Yu added a comment -

        Patch v11 adds protection to other methods of CallQueue.java

        Show
        Ted Yu added a comment - Patch v11 adds protection to other methods of CallQueue.java
        Ted Yu made changes -
        Attachment 5186-v11.txt [ 12512887 ]
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Cool. If they all use locking as concurrency control, adding one extra lock will not change the performance much.

        v11 looks good to me. I only have one question. I notice that there is no lock for add, remove, addAll, containsAll, retainAll and removeAll in v11. Is that intentional?

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Cool. If they all use locking as concurrency control, adding one extra lock will not change the performance much. v11 looks good to me. I only have one question. I notice that there is no lock for add, remove, addAll, containsAll, retainAll and removeAll in v11. Is that intentional? REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512887/5186-v11.txt
        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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 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.thrift.TestThriftServerCmdLine

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/891//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/891//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/891//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/12512887/5186-v11.txt 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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 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.thrift.TestThriftServerCmdLine Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/891//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/891//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/891//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 I searched HBase code base and didn't find any call to drainTo().

        For simplicity and correctness, I think patch v8 looks better.

        If drainTo() becomes performance bottleneck, we can come back optimizing it.

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

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 I searched HBase code base and didn't find any call to drainTo(). For simplicity and correctness, I think patch v8 looks better. If drainTo() becomes performance bottleneck, we can come back optimizing it. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Ted: I agree. I think ExecutorService only do put() and take() to this queue. It's better to keep things simple. Thanks for the effort for optimizing the code

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Ted: I agree. I think ExecutorService only do put() and take() to this queue. It's better to keep things simple. Thanks for the effort for optimizing the code REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has commented on the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Just checked openjdk ThreadPoolExecutor.java
        http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/concurrent/ThreadPoolExecutor.java#ThreadPoolExecutor

        drainTo() is called by drainQueue() which is called only in shutdownNow(). So I guess you are right about that we can leave this for now.

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

        Show
        Phabricator added a comment - sc has commented on the revision " HBASE-5186 [jira] Add metrics to ThriftServer". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java:131 Just checked openjdk ThreadPoolExecutor.java http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/concurrent/ThreadPoolExecutor.java#ThreadPoolExecutor drainTo() is called by drainQueue() which is called only in shutdownNow(). So I guess you are right about that we can leave this for now. REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Ted Yu added a comment -

        Patch v12 adds logging to CallQueue.java

        Show
        Ted Yu added a comment - Patch v12 adds logging to CallQueue.java
        Ted Yu made changes -
        Attachment 5186-v12.txt [ 12512964 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512964/5186-v12.txt
        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 appears to have generated -140 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 157 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.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/895//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/895//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/895//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/12512964/5186-v12.txt 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 appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 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.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/895//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/895//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/895//console This message is automatically generated.
        Ted Yu made changes -
        Hadoop Flags Reviewed [ 10343 ]
        Fix Version/s 0.94.0 [ 12316419 ]
        Hide
        Ted Yu added a comment -

        Integrated to TRUNK.

        Thanks for the patch Scott.

        Show
        Ted Yu added a comment - Integrated to TRUNK. Thanks for the patch Scott.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2652 (See https://builds.apache.org/job/HBase-TRUNK/2652/)
        HBASE-5186 Add metrics to ThriftServer (Scott Chen)

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2652 (See https://builds.apache.org/job/HBase-TRUNK/2652/ ) HBASE-5186 Add metrics to ThriftServer (Scott Chen) tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #96 (See https://builds.apache.org/job/HBase-TRUNK-security/96/)
        HBASE-5186 Add metrics to ThriftServer (Scott Chen)

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #96 (See https://builds.apache.org/job/HBase-TRUNK-security/96/ ) HBASE-5186 Add metrics to ThriftServer (Scott Chen) tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/CallQueue.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestCallQueue.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
        Hide
        Scott Chen added a comment -

        Ted: Thanks for the review on this patch. That helped a lot.

        Show
        Scott Chen added a comment - Ted: Thanks for the review on this patch. That helped a lot.
        Ted Yu made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Phabricator added a comment -

        dhruba has accepted the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

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

        Show
        Phabricator added a comment - dhruba has accepted the revision " HBASE-5186 [jira] Add metrics to ThriftServer". REVISION DETAIL https://reviews.facebook.net/D1461
        Hide
        Phabricator added a comment -

        sc has closed the revision "HBASE-5186 [jira] Add metrics to ThriftServer".

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

        To: dhruba, tedyu, JIRA, heyongqiang, sc
        Cc: tedyu, sc, dhruba

        Show
        Phabricator added a comment - sc has closed the revision " HBASE-5186 [jira] Add metrics to ThriftServer". REVISION DETAIL https://reviews.facebook.net/D1461 To: dhruba, tedyu, JIRA, heyongqiang, sc Cc: tedyu, sc, dhruba
        Lars Hofhansl made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Scott Chen
            Reporter:
            Scott Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development