HBase
  1. HBase
  2. HBASE-4863

Make Thrift server thread pool bounded and add a command-line UI test

    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

      This started as an internal hotfix where we found out that the Thrift server spawned 15000 threads. To bound the thread pool size I added a custom thread pool server implementation called HBaseThreadPoolServer into HBase codebase, and made the following parameters configurable from both command line and as config settings: minWorkerThreads, maxWorkerThreads, and maxQueuedRequests. Under an increasing load, the server creates new threads for every connection before the pool size reaches minWorkerThreads. After that, the server puts new connections into the queue and only creates a new thread when the queue is full. If an attempt to create a new thread fails, the server drops connection. The default TThreadPoolServer would crash in that case, but it never happened because the thread pool was unbounded, so the server would hang indefinitely, consume a lot of memory, and cause huge latency spikes on the client side.

      Another part of this fix is refactoring and unit testing of the command-line part of the Thrift server. The logic there is sufficiently complicated, and the existing ThriftServer class does not test that part at all. The new TestThriftServerCmdLine test starts the Thrift server on a random port with various combinations of options and talks to it through the client API from another thread.

      1. ASF.LICENSE.NOT.GRANTED--D531.1.patch
        50 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--D531.2.patch
        50 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--D531.3.patch
        55 kB
        Phabricator
      4. 0001-Fix-thread-leaks-in-the-HBase-thread-pool-server.patch
        58 kB
        Mikhail Bautin
      5. 0002-Fix-thread-leaks-in-the-HBase-thread-pool-server.patch
        58 kB
        Mikhail Bautin
      6. ASF.LICENSE.NOT.GRANTED--D531.4.patch
        55 kB
        Phabricator
      7. 4863.addendum
        0.8 kB
        Ted Yu

        Issue Links

          Activity

          Hide
          Phabricator added a comment -

          mbautin has abandoned the revision "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".

          This diff was committed by @TedYu (http://svn.apache.org/repos/asf/hbase/trunk@1206267, http://svn.apache.org/repos/asf/hbase/trunk@1206369).

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

          Show
          Phabricator added a comment - mbautin has abandoned the revision " [jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test". This diff was committed by @TedYu ( http://svn.apache.org/repos/asf/hbase/trunk@1206267 , http://svn.apache.org/repos/asf/hbase/trunk@1206369 ). REVISION DETAIL https://reviews.facebook.net/D531
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #10 (See https://builds.apache.org/job/HBase-TRUNK-security/10/)
          HBASE-4863 Addendum to add category for TestThreads
          HBASE-4863 Phabricator D531 Make Thrift server thread pool bounded and add a command-line UI test

          tedyu :
          Files :

          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java

          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/Threads.java
          • /hbase/trunk/src/main/resources/hbase-default.xml
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #10 (See https://builds.apache.org/job/HBase-TRUNK-security/10/ ) HBASE-4863 Addendum to add category for TestThreads HBASE-4863 Phabricator D531 Make Thrift server thread pool bounded and add a command-line UI test tedyu : Files : /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/Threads.java /hbase/trunk/src/main/resources/hbase-default.xml /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2484 (See https://builds.apache.org/job/HBase-TRUNK/2484/)
          HBASE-4863 Addendum to add category for TestThreads

          tedyu :
          Files :

          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2484 (See https://builds.apache.org/job/HBase-TRUNK/2484/ ) HBASE-4863 Addendum to add category for TestThreads tedyu : Files : /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Hide
          Ted Yu added a comment -

          Addendum applied to TRUNK.

          Show
          Ted Yu added a comment - Addendum applied to TRUNK.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12505163/4863.addendum
          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/372//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/12505163/4863.addendum 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/372//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Addendum to add category for TestThreads

          Show
          Ted Yu added a comment - Addendum to add category for TestThreads
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2482 (See https://builds.apache.org/job/HBase-TRUNK/2482/)
          HBASE-4863 Phabricator D531 Make Thrift server thread pool bounded and add a command-line UI test

          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/Threads.java
          • /hbase/trunk/src/main/resources/hbase-default.xml
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2482 (See https://builds.apache.org/job/HBase-TRUNK/2482/ ) HBASE-4863 Phabricator D531 Make Thrift server thread pool bounded and add a command-line UI test tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/Threads.java /hbase/trunk/src/main/resources/hbase-default.xml /hbase/trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Hide
          Ted Yu added a comment -

          Integrated to TRUNK.

          Thanks for the patch Mikhail.

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

          +1 on patch v2.
          The test failures reported by HadoopQA weren't related to the patch.

          Show
          Ted Yu added a comment - +1 on patch v2. The test failures reported by HadoopQA weren't related to the patch.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 javadoc. The javadoc tool appears to have generated -162 warning messages.

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

          -1 findbugs. The patch appears to introduce 67 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.TestReplication
          org.apache.hadoop.hbase.client.TestInstantSchemaChange

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/366//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/366//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/366//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/12505097/D531.4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -162 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 67 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.TestReplication org.apache.hadoop.hbase.client.TestInstantSchemaChange Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/366//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/366//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/366//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".
          Reviewers: JIRA, Kannan, tedyu, stack

          Fixing a bug in TestThreads. Cluster testing is going well. I will kick off a unit test run on Jenkins.

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
          src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/main/resources/hbase-default.xml
          src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
          src/test/java/org/apache/hadoop/hbase/util/TestThreads.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test". Reviewers: JIRA, Kannan, tedyu, stack Fixing a bug in TestThreads. Cluster testing is going well. I will kick off a unit test run on Jenkins. REVISION DETAIL https://reviews.facebook.net/D531 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/main/resources/hbase-default.xml src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Hide
          Ted Yu added a comment -
          testSleepWithoutInterrupt(org.apache.hadoop.hbase.util.TestThreads)  Time elapsed: 5.004 sec  <<< FAILURE!
          java.lang.AssertionError
            at org.junit.Assert.fail(Assert.java:92)
            at org.junit.Assert.assertTrue(Assert.java:43)
            at org.junit.Assert.assertTrue(Assert.java:54)
            at org.apache.hadoop.hbase.util.TestThreads.testSleepWithoutInterrupt(TestThreads.java:57)
          

          points to this line:

                assertTrue(sleeper.isInterrupted());
          
          Show
          Ted Yu added a comment - testSleepWithoutInterrupt(org.apache.hadoop.hbase.util.TestThreads) Time elapsed: 5.004 sec <<< FAILURE! java.lang.AssertionError at org.junit.Assert.fail(Assert.java:92) at org.junit.Assert.assertTrue(Assert.java:43) at org.junit.Assert.assertTrue(Assert.java:54) at org.apache.hadoop.hbase.util.TestThreads.testSleepWithoutInterrupt(TestThreads.java:57) points to this line: assertTrue(sleeper.isInterrupted());
          Hide
          Ted Yu added a comment -

          In thrift2/ThriftServer.java:

                } else {
                  server = getTThreadPoolServer(protocolFactory, processor, transportFactory, inetSocketAddress);
          

          where

              TThreadPoolServer.Args serverArgs = new TThreadPoolServer.Args(serverTransport);
          

          It would be nice to incorporate TBoundedThreadPoolServer into the above module. This can be done in a separate JIRA.

          Show
          Ted Yu added a comment - In thrift2/ThriftServer.java: } else { server = getTThreadPoolServer(protocolFactory, processor, transportFactory, inetSocketAddress); where TThreadPoolServer.Args serverArgs = new TThreadPoolServer.Args(serverTransport); It would be nice to incorporate TBoundedThreadPoolServer into the above module. This can be done in a separate JIRA.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12505038/0001-Fix-thread-leaks-in-the-HBase-thread-pool-server.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -162 warning messages.

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

          -1 findbugs. The patch appears to introduce 67 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.util.TestThreads

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/364//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/364//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/364//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/12505038/0001-Fix-thread-leaks-in-the-HBase-thread-pool-server.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -162 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 67 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.util.TestThreads Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/364//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/364//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/364//console This message is automatically generated.
          Hide
          Mikhail Bautin added a comment -

          The same as D531.3.patch but generated using "git format-patch --no-prefix HEAD^..HEAD" so that it can be applied using the normal patch command.

          Show
          Mikhail Bautin added a comment - The same as D531.3.patch but generated using "git format-patch --no-prefix HEAD^..HEAD" so that it can be applied using the normal patch command.
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".
          Reviewers: JIRA, Kannan, tedyu, stack

          Addressing Ted's comments. I will re-run unit tests and cluster tests, and post an update.

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java
          src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/main/resources/hbase-default.xml
          src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
          src/test/java/org/apache/hadoop/hbase/util/TestThreads.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test". Reviewers: JIRA, Kannan, tedyu, stack Addressing Ted's comments. I will re-run unit tests and cluster tests, and post an update. REVISION DETAIL https://reviews.facebook.net/D531 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/TBoundedThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/main/resources/hbase-default.xml src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".

          Should similar changes in thrift/ThriftServer.java be applied to thrift2/ThriftServer.java ?

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:111 Should this become a parameter user can adjust ?
          src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:263 Should ttx.getType() be logged ?
          src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java:179 Should read 'Exactly one '

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test". Should similar changes in thrift/ThriftServer.java be applied to thrift2/ThriftServer.java ? INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:111 Should this become a parameter user can adjust ? src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:263 Should ttx.getType() be logged ? src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java:179 Should read 'Exactly one ' REVISION DETAIL https://reviews.facebook.net/D531
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:64 Please add javadoc for the keys.
          These keys should be placed into hbase-default.xml
          src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:80 Is TIME_TO_WAIT_AFTER_SHUTDOWN_MS a better name for this constant ?

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

          Show
          Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:64 Please add javadoc for the keys. These keys should be placed into hbase-default.xml src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java:80 Is TIME_TO_WAIT_AFTER_SHUTDOWN_MS a better name for this constant ? REVISION DETAIL https://reviews.facebook.net/D531
          Hide
          Ted Yu added a comment - - edited

          I got compilation error:

          testRunThriftServer[0](org.apache.hadoop.hbase.thrift.TestThriftServerCmdLine)  Time elapsed: 2.047 sec  <<< ERROR!
          java.lang.Error: Unresolved compilation problem:
            Cannot make a static reference to the non-static method getColumnDescriptors() from the type TestThriftServer
          
            at org.apache.hadoop.hbase.thrift.TestThriftServer.createDropTable(TestThriftServer.java:111)
          

          Since HBaseThreadPoolServer extends TServer, I think a better name for the class would be TBoundedThreadPoolServer (TThreadPoolServer is in thrift).

          Show
          Ted Yu added a comment - - edited I got compilation error : testRunThriftServer[0](org.apache.hadoop.hbase.thrift.TestThriftServerCmdLine) Time elapsed: 2.047 sec <<< ERROR! java.lang.Error: Unresolved compilation problem: Cannot make a static reference to the non- static method getColumnDescriptors() from the type TestThriftServer at org.apache.hadoop.hbase.thrift.TestThriftServer.createDropTable(TestThriftServer.java:111) Since HBaseThreadPoolServer extends TServer, I think a better name for the class would be TBoundedThreadPoolServer (TThreadPoolServer is in thrift).
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".
          Reviewers: JIRA, Kannan, tedyu, stack

          Updating with the most recent version. Posted a stale version at first – sorry for spam.

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java
          src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
          src/test/java/org/apache/hadoop/hbase/util/TestThreads.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test". Reviewers: JIRA, Kannan, tedyu, stack Updating with the most recent version. Posted a stale version at first – sorry for spam. REVISION DETAIL https://reviews.facebook.net/D531 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java src/test/java/org/apache/hadoop/hbase/util/TestThreads.java
          Hide
          Phabricator added a comment -

          mbautin requested code review of "[jira] HBASE-4863 Make HBase Thrift server more configurable and add a command-line UI test".
          Reviewers: JIRA, Kannan, tedyu, stack

          This started as an internal hotfix where we found out that the Thrift server spawned 15000 threads. To bound the thread pool size I added a custom thread pool server implementation called HBaseThreadPoolServer into HBase codebase, and made the following parameters configurable from both command line and as config settings: minWorkerThreads, maxWorkerThreads, and maxQueuedRequests. Under an increasing load, the server creates new threads for every connection before the pool size reaches minWorkerThreads. After that, the server puts new connections into the queue and only creates a new thread when the queue is full. If an attempt to create a new thread fails, the server drops connection. The default TThreadPoolServer would crash in that case, but it never happened because the thread pool was unbounded, so the server would hang indefinitely, consume a lot of memory, and cause huge latency spikes on the client side.

          Another part of this fix is refactoring and unit testing of the command-line part of the Thrift server. The logic there is sufficiently complicated, and the existing ThriftServer class does not test that part at all. The new TestThriftServerCmdLine test starts the Thrift server on a random port with various combinations of options and talks to it through the client API from another thread.

          TEST PLAN
          Unit tests, cluster test with a Python Thrift client.
          I will post an update when I'm done with testing.

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

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java
          src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
          src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
          src/test/java/org/apache/hadoop/hbase/util/TestThreads.java

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

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

          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-4863 Make HBase Thrift server more configurable and add a command-line UI test". Reviewers: JIRA, Kannan, tedyu, stack This started as an internal hotfix where we found out that the Thrift server spawned 15000 threads. To bound the thread pool size I added a custom thread pool server implementation called HBaseThreadPoolServer into HBase codebase, and made the following parameters configurable from both command line and as config settings: minWorkerThreads, maxWorkerThreads, and maxQueuedRequests. Under an increasing load, the server creates new threads for every connection before the pool size reaches minWorkerThreads. After that, the server puts new connections into the queue and only creates a new thread when the queue is full. If an attempt to create a new thread fails, the server drops connection. The default TThreadPoolServer would crash in that case, but it never happened because the thread pool was unbounded, so the server would hang indefinitely, consume a lot of memory, and cause huge latency spikes on the client side. Another part of this fix is refactoring and unit testing of the command-line part of the Thrift server. The logic there is sufficiently complicated, and the existing ThriftServer class does not test that part at all. The new TestThriftServerCmdLine test starts the Thrift server on a random port with various combinations of options and talks to it through the client API from another thread. TEST PLAN Unit tests, cluster test with a Python Thrift client. I will post an update when I'm done with testing. REVISION DETAIL https://reviews.facebook.net/D531 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/thrift/HBaseThreadPoolServer.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java src/test/java/org/apache/hadoop/hbase/util/TestThreads.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/1167/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development