Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.99.0, 2.0.0, 0.98.6
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      Adds "hbase.regionstatestore.meta.connection" configuration and new MultiHConnection class. Is set to 1 by default. Set it higher if you want to run with more than one connection to the meta table. Useful if you have a very large meta table -- e.g. 1M regions -- otherwise, stick to the default.
      Show
      Adds "hbase.regionstatestore.meta.connection" configuration and new MultiHConnection class. Is set to 1 by default. Set it higher if you want to run with more than one connection to the meta table. Useful if you have a very large meta table -- e.g. 1M regions -- otherwise, stick to the default.

      Description

      Currently, if the meta region is on a regionserver instead of the master, meta update is synchronized on one HTable instance. We should be able to do better.

      1. HBASE-11610-branch1.patch
        15 kB
        Virag Kothari
      2. HBASE-11610.patch
        9 kB
        Virag Kothari
      3. HBASE-11610_v4.patch
        15 kB
        Virag Kothari
      4. HBASE-11610_v3.patch
        15 kB
        Virag Kothari
      5. HBASE-11610_2.patch
        12 kB
        Virag Kothari

        Issue Links

          Activity

          Hide
          Enis Soztutar added a comment -

          Closing this issue after 0.99.0 release.

          Show
          Enis Soztutar added a comment - Closing this issue after 0.99.0 release.
          Hide
          Andrew Purtell added a comment -

          Pushed to 0.98 as part of HBASE-11546

          Show
          Andrew Purtell added a comment - Pushed to 0.98 as part of HBASE-11546
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-1.0 #124 (See https://builds.apache.org/job/HBase-1.0/124/)
          HBASE-11610 Enhance remote meta updates (Virag Kothari) (stack: rev 59230cb1847923c98c7d08deb8b8b56830ab785a)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionStateStore.java
          • hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerFactory.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java
            HBASE-11610 Enhance remote meta updates (Virag Kothari) (stack: rev b1b038a77103d19d432f882c61cfb137fcf2c7c5)
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/MultiHConnection.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-1.0 #124 (See https://builds.apache.org/job/HBase-1.0/124/ ) HBASE-11610 Enhance remote meta updates (Virag Kothari) (stack: rev 59230cb1847923c98c7d08deb8b8b56830ab785a) hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionStateStore.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerFactory.java hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java HBASE-11610 Enhance remote meta updates (Virag Kothari) (stack: rev b1b038a77103d19d432f882c61cfb137fcf2c7c5) hbase-server/src/main/java/org/apache/hadoop/hbase/util/MultiHConnection.java
          Hide
          stack added a comment -

          Applied to branch-1. Hope that good w/ you Enis Soztutar

          Show
          stack added a comment - Applied to branch-1. Hope that good w/ you Enis Soztutar
          Hide
          Virag Kothari added a comment -

          Thanks for the reviews Jimmy, Nicolas and Stack.

          stack Patch for branch-1 attached. Please commit. Thanks!
          The 0.98 patch is attached to HBASE-11546. Andrew will commit that one along with the zk less backport.

          Show
          Virag Kothari added a comment - Thanks for the reviews Jimmy, Nicolas and Stack. stack Patch for branch-1 attached. Please commit. Thanks! The 0.98 patch is attached to HBASE-11546 . Andrew will commit that one along with the zk less backport.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-TRUNK #5425 (See https://builds.apache.org/job/HBase-TRUNK/5425/)
          HBASE-11610 Enhance remote meta updates (stack: rev dd6c21e4d5bdc4e3b733ff25c427bba4a224242b)

          • hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerFactory.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionStateStore.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/MultiHConnection.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #5425 (See https://builds.apache.org/job/HBase-TRUNK/5425/ ) HBASE-11610 Enhance remote meta updates (stack: rev dd6c21e4d5bdc4e3b733ff25c427bba4a224242b) hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerFactory.java hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionStateStore.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/MultiHConnection.java
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12664173/HBASE-11610_v4.patch
          against trunk revision .
          ATTACHMENT ID: 12664173

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

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

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

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

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.TestRegionRebalancing

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//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/12664173/HBASE-11610_v4.patch against trunk revision . ATTACHMENT ID: 12664173 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 6 warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestRegionRebalancing Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10568//console This message is automatically generated.
          Hide
          stack added a comment -

          Committed to 2.0 branch. It does not go into 1.0 nor 0.98 clean. If you make a patch for these branches Virag Kothari I'll aply there np.

          Show
          stack added a comment - Committed to 2.0 branch. It does not go into 1.0 nor 0.98 clean. If you make a patch for these branches Virag Kothari I'll aply there np.
          Hide
          Virag Kothari added a comment -

          Attached is the patch which addresses all comments.

          Show
          Virag Kothari added a comment - Attached is the patch which addresses all comments.
          Hide
          Virag Kothari added a comment -

          new patch, fix it

          Sure.

          There is just one pool going on

          Yes. Only one. Just now checked from few jstacks I had from last time

          Show
          Virag Kothari added a comment - new patch, fix it Sure. There is just one pool going on Yes. Only one. Just now checked from few jstacks I had from last time
          Hide
          stack added a comment -

          License on MultiHC is oddly formatted. If you are making a new patch, fix it.

          On the new pool, for sure we are not putting up an excess of threads in client? This has been an issue in the past. There is just one pool going on? You've taken a look at a running client Virag Kothari?

          Otherwise, patch lgtm

          Show
          stack added a comment - License on MultiHC is oddly formatted. If you are making a new patch, fix it. On the new pool, for sure we are not putting up an excess of threads in client? This has been an issue in the past. There is just one pool going on? You've taken a look at a running client Virag Kothari ? Otherwise, patch lgtm
          Hide
          Nicolas Liochon added a comment - - edited

          threadLocal HTable ), it is ~5% slower

          Ah, ok. It's a little bit strange actually. But not big enough to delay the patch.

          done with 10 HConnections

          I see. Then let's keep it.

          will use the single pool managed by MultiHConnection

          I haven't checked in the code, but it seems you're right.

          how about hbase.multihconnection.threads.max as the pool is part of MultiHconnection?

          No strong opinion. As you like for me.

          +1 for the patch, as the remaining is pure nit.

          Show
          Nicolas Liochon added a comment - - edited threadLocal HTable ), it is ~5% slower Ah, ok. It's a little bit strange actually. But not big enough to delay the patch. done with 10 HConnections I see. Then let's keep it. will use the single pool managed by MultiHConnection I haven't checked in the code, but it seems you're right. how about hbase.multihconnection.threads.max as the pool is part of MultiHconnection? No strong opinion. As you like for me. +1 for the patch, as the remaining is pure nit.
          Hide
          Virag Kothari added a comment -

          It's great that v3 is both simpler and more efficient!

          Its ~5% faster than v2 which creates a seperate HTable for each put. Compared to the threadLocal HTable (original patch), it is ~5% slower. (this experiments are done over series of 5 runs).

          is MultiHConnection that useful? I see its size is defaulted to 1? have you compare the performances with greater values?

          All the above experiments are done with 10 HConnections. It improves the write throughput on META server by ~15% compared to single HConnection when doing assignment for 1M regions. But most users will have significantly lower than 1M regions, so by default we only will have one HConnection.
          I had tried with 15 connections but didn't see any increase in throughput.

          Each Connection will come with a pool of 256 threads (there is HBASE-11590 to improve this a little), plus the one of MultiHConnection

          I believe that pool is only created when you do a getTable() on HConnection. As we are not going the HTable route, all threads accessing regionStateStore will use the single pool managed by MultiHConnection. We can get rid of this pool when we have a new processBatchCallBack API which maintains a pool (though that will be per connection).

          we should have some thing like hbase.regionstatestore.meta.threads.max instead of hbase.hconnection.threads.max)

          how about hbase.multihconnection.threads.max as the pool is part of MultiHconnection?

          I like the patch

          Thanks!

          Will address comments shortly.

          Show
          Virag Kothari added a comment - It's great that v3 is both simpler and more efficient! Its ~5% faster than v2 which creates a seperate HTable for each put. Compared to the threadLocal HTable (original patch), it is ~5% slower. (this experiments are done over series of 5 runs). is MultiHConnection that useful? I see its size is defaulted to 1? have you compare the performances with greater values? All the above experiments are done with 10 HConnections. It improves the write throughput on META server by ~15% compared to single HConnection when doing assignment for 1M regions. But most users will have significantly lower than 1M regions, so by default we only will have one HConnection. I had tried with 15 connections but didn't see any increase in throughput. Each Connection will come with a pool of 256 threads (there is HBASE-11590 to improve this a little), plus the one of MultiHConnection I believe that pool is only created when you do a getTable() on HConnection. As we are not going the HTable route, all threads accessing regionStateStore will use the single pool managed by MultiHConnection. We can get rid of this pool when we have a new processBatchCallBack API which maintains a pool (though that will be per connection). we should have some thing like hbase.regionstatestore.meta.threads.max instead of hbase.hconnection.threads.max) how about hbase.multihconnection.threads.max as the pool is part of MultiHconnection? I like the patch Thanks! Will address comments shortly.
          Hide
          Nicolas Liochon added a comment -

          It's great that v3 is both simpler and more efficient!

          I like the fact that all the complexity is put in a different class.

          MultiHConnection should be annotated private
          (nit)access to multiHConnection in #stop should be synchronized to be sure we see the last value.

          is MultiHConnection that useful? I see its size is defaulted to 1? have you compare the performances with greater values? Each Connection will come with a pool of 256 threads (there is HBASE-11590 to improve this a little), plus the one of MultiHConnection... As well, may be the TP in MultiHConnection should be configurable independently of the HConnection TP (i.e. we should have some thing like hbase.regionstatestore.meta.threads.max instead of hbase.hconnection.threads.max). But, may be the simplest is just to remove all the MultiHConnection (less code to maintain...)

          return this.batchPool = tpe;

          (nit) This would be more readable with two lines.

          All these comments are more or less nit stuff. I like the patch.

          Show
          Nicolas Liochon added a comment - It's great that v3 is both simpler and more efficient! I like the fact that all the complexity is put in a different class. MultiHConnection should be annotated private (nit)access to multiHConnection in #stop should be synchronized to be sure we see the last value. is MultiHConnection that useful? I see its size is defaulted to 1? have you compare the performances with greater values? Each Connection will come with a pool of 256 threads (there is HBASE-11590 to improve this a little), plus the one of MultiHConnection... As well, may be the TP in MultiHConnection should be configurable independently of the HConnection TP (i.e. we should have some thing like hbase.regionstatestore.meta.threads.max instead of hbase.hconnection.threads.max). But, may be the simplest is just to remove all the MultiHConnection (less code to maintain...) return this.batchPool = tpe; (nit) This would be more readable with two lines. All these comments are more or less nit stuff. I like the patch.
          Hide
          Virag Kothari added a comment -

          Stack/Nicolas Liochon, Can v3 be reviewed? As zk less assignment backport (HBASE-11546) is going in 0.98.6, I would like this one to go in too. Thanks!

          Show
          Virag Kothari added a comment - Stack / Nicolas Liochon , Can v3 be reviewed? As zk less assignment backport ( HBASE-11546 ) is going in 0.98.6, I would like this one to go in too. Thanks!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12663821/HBASE-11610_v3.patch
          against trunk revision .
          ATTACHMENT ID: 12663821

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

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

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

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

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.client.TestTableSnapshotScanner

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//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/12663821/HBASE-11610_v3.patch against trunk revision . ATTACHMENT ID: 12663821 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 6 warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestTableSnapshotScanner Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10549//console This message is automatically generated.
          Hide
          Virag Kothari added a comment -

          v3 uses HConnection.processBatchCallback() and some improvement (~5%) over v2 which uses Htable.
          Please review. Thanks

          Show
          Virag Kothari added a comment - v3 uses HConnection.processBatchCallback() and some improvement (~5%) over v2 which uses Htable. Please review. Thanks
          Hide
          Virag Kothari added a comment -

          Nicolas Liochon I will create a JIRA to further the discussion around undeprecation of processBatch(). Thanks for your input

          Show
          Virag Kothari added a comment - Nicolas Liochon I will create a JIRA to further the discussion around undeprecation of processBatch(). Thanks for your input
          Hide
          Virag Kothari added a comment -

          Updated patch removes ThreadLocalHTable and addresses other comments.
          Also added a small change to avoid reflection call (in most cases) to instantiate RpcRetryingCallerFactory during creation of Htable instance.

          Show
          Virag Kothari added a comment - Updated patch removes ThreadLocalHTable and addresses other comments. Also added a small change to avoid reflection call (in most cases) to instantiate RpcRetryingCallerFactory during creation of Htable instance.
          Hide
          Nicolas Liochon added a comment -

          I think we should have an additional processBatch which takes care of that

          I agree.

          Should we have a JIRA to discuss all this?

          As you like. It's not necessary imho, we can do that here. Cleaning up all stuff around thread pools would take some time. This jira is a good input for this work.

          Show
          Nicolas Liochon added a comment - I think we should have an additional processBatch which takes care of that I agree. Should we have a JIRA to discuss all this? As you like. It's not necessary imho, we can do that here. Cleaning up all stuff around thread pools would take some time. This jira is a good input for this work.
          Hide
          Virag Kothari added a comment -

          It's more or less the same code that is used behind the scene when you do a single put.

          I see

          it's better it justifies an undeprecation imho

          Agree. This case does justify the un-deprecation.
          One issue currently with the processBatch() API is that one has to separately pass a pool. If I want to use it, I would have to duplicate the logic of HconnectionManager.getBatchPool(). I think we should have an additional processBatch API which takes care of that. Should we have a JIRA to discuss all this?

          Show
          Virag Kothari added a comment - It's more or less the same code that is used behind the scene when you do a single put. I see it's better it justifies an undeprecation imho Agree. This case does justify the un-deprecation. One issue currently with the processBatch() API is that one has to separately pass a pool. If I want to use it, I would have to duplicate the logic of HconnectionManager.getBatchPool(). I think we should have an additional processBatch API which takes care of that. Should we have a JIRA to discuss all this?
          Hide
          Nicolas Liochon added a comment -

          , this method seems to be more helpful if there is a batch of Puts.

          It's more or less the same code that is used behind the scene when you do a single put.

          I hope this works for you guys?

          Strong +1 for not using the ThreadLocalHtable. Personally I would give a try to the call to HConnection#processBatch stuff. It's it's better it justifies an undeprecation imho (I'm not saying this just for this jira: I think it make sense to have an interface different than HTable). But if you prefer another approach, it's fine for me, it's your patch (provided that there is no thread local )

          Show
          Nicolas Liochon added a comment - , this method seems to be more helpful if there is a batch of Puts. It's more or less the same code that is used behind the scene when you do a single put. I hope this works for you guys? Strong +1 for not using the ThreadLocalHtable. Personally I would give a try to the call to HConnection#processBatch stuff. It's it's better it justifies an undeprecation imho (I'm not saying this just for this jira: I think it make sense to have an interface different than HTable). But if you prefer another approach, it's fine for me, it's your patch (provided that there is no thread local )
          Hide
          stack added a comment -

          This scenario is explained in the comment few lines above

          Sorry. Missed that.

          Show
          stack added a comment - This scenario is explained in the comment few lines above Sorry. Missed that.
          Hide
          Virag Kothari added a comment -

          Thanks for the review.
          Based on general feedback here, I will remove the ThreadLocalHtable. Creating a htable for each put might produce more garbage, but guess that should be fine given that htable is lightweight. We can still have multiple HConnections to have a higher write throughput, but can get a HTable instance for a connection picked randomly. I hope this works for you guys?
          Will do a performance run to check the memory utilization and see if there is any performance degradation with this approach.

          I'm not a big fan of the ThreadLocal<HTableInterface> threadLocalHTable. It's often difficult to maintain and test.

          Agree its difficult to debug threadlocal issues.
          As you said that HConnection.processBatchCallback() is deprecated, so am not very inclined to use that. Also, this method seems to be more helpful if there is a batch of Puts.

          Will address Stack's comments of changing the config name, separating the connection setup part in a different utility class and adding more docs/comments.

          i.e. when would hConnectionPool be null?

          This is only during test cases when meta remains no longer on master. This scenario is explained in the comment few lines above.

          Show
          Virag Kothari added a comment - Thanks for the review. Based on general feedback here, I will remove the ThreadLocalHtable. Creating a htable for each put might produce more garbage, but guess that should be fine given that htable is lightweight. We can still have multiple HConnections to have a higher write throughput, but can get a HTable instance for a connection picked randomly. I hope this works for you guys? Will do a performance run to check the memory utilization and see if there is any performance degradation with this approach. I'm not a big fan of the ThreadLocal<HTableInterface> threadLocalHTable. It's often difficult to maintain and test. Agree its difficult to debug threadlocal issues. As you said that HConnection.processBatchCallback() is deprecated, so am not very inclined to use that. Also, this method seems to be more helpful if there is a batch of Puts. Will address Stack's comments of changing the config name, separating the connection setup part in a different utility class and adding more docs/comments. i.e. when would hConnectionPool be null? This is only during test cases when meta remains no longer on master. This scenario is explained in the comment few lines above.
          Hide
          Nicolas Liochon added a comment -

          The perf improvement is great.
          I'm not a big fan of the ThreadLocal<HTableInterface> threadLocalHTable. It's often difficult to maintain and test.

          If I understand well the issue if that the put is synchronous, and all the threads are queueing?

          Should we use something like

          HConnection#processBatchCallback(List<? extends Row> list,
          final TableName tableName,
          ExecutorService pool,
          Object[] results,
          Batch.Callback<R> callback) throws IOException, InterruptedException;

          instead of using HTable? HConnection is thread safe, so there is no sync needed.
          (ok it's deprecated, but if it saves this kind of hack may be we need to review our point of view).

          Show
          Nicolas Liochon added a comment - The perf improvement is great. I'm not a big fan of the ThreadLocal<HTableInterface> threadLocalHTable. It's often difficult to maintain and test. If I understand well the issue if that the put is synchronous, and all the threads are queueing? Should we use something like HConnection#processBatchCallback(List<? extends Row> list, final TableName tableName, ExecutorService pool, Object[] results, Batch.Callback<R> callback) throws IOException, InterruptedException; instead of using HTable? HConnection is thread safe, so there is no sync needed. (ok it's deprecated, but if it saves this kind of hack may be we need to review our point of view).
          Hide
          stack added a comment -

          This patch is a bit of a hack. We are doing a one-off inside RegionStateStore to put up multiple HConnection instances (for sure we are creating many distinct instances?). I'd doubt anyone but you fellas will know of its existance (Needs a release not on the new config, "hbase.statestore.meta.connection", and new config should probably be called "hbase.regionstatestore.meta.connection"). Would be nice if this connection setup was off in a separate class so should anyone else want to do this trick, they'll not duplicate your effort. This is just a nit though. I'm also fine with adding in stuff that is custom for you fellas (custom for now) just as long as it is well doc'd.

          When would this code trigger?

          + if (hConnectionPool == null) {
          + hConnectionPool = new HConnection[]

          {HConnectionManager.createConnection(server.getConfiguration())}

          ;
          + }

          i.e. when would hConnectionPool be null?

          Should this be private? + private ThreadLocal<HTableInterface> threadLocalHTable =

          It should have a comment on when this thread local gets instantiated – what the current thread is at the time.

          Show
          stack added a comment - This patch is a bit of a hack. We are doing a one-off inside RegionStateStore to put up multiple HConnection instances (for sure we are creating many distinct instances?). I'd doubt anyone but you fellas will know of its existance (Needs a release not on the new config, "hbase.statestore.meta.connection", and new config should probably be called "hbase.regionstatestore.meta.connection"). Would be nice if this connection setup was off in a separate class so should anyone else want to do this trick, they'll not duplicate your effort. This is just a nit though. I'm also fine with adding in stuff that is custom for you fellas (custom for now) just as long as it is well doc'd. When would this code trigger? + if (hConnectionPool == null) { + hConnectionPool = new HConnection[] {HConnectionManager.createConnection(server.getConfiguration())} ; + } i.e. when would hConnectionPool be null? Should this be private? + private ThreadLocal<HTableInterface> threadLocalHTable = It should have a comment on when this thread local gets instantiated – what the current thread is at the time.
          Hide
          Jimmy Xiang added a comment -

          I have no more comment. I am ok with a patch that shares just one HConnection and one execution pool, closes the meta htable instance after each use. I am not sure about the current patch. Perhaps larsh/Nicolas Liochon can take a look?

          Show
          Jimmy Xiang added a comment - I have no more comment. I am ok with a patch that shares just one HConnection and one execution pool, closes the meta htable instance after each use. I am not sure about the current patch. Perhaps larsh / Nicolas Liochon can take a look?
          Hide
          Virag Kothari added a comment -

          Jimmy Xiang Any other comments before we can get this in?

          Show
          Virag Kothari added a comment - Jimmy Xiang Any other comments before we can get this in?
          Hide
          Jimmy Xiang added a comment -

          I see. It's better to use connection.getHTable().

          Show
          Jimmy Xiang added a comment - I see. It's better to use connection.getHTable().
          Hide
          Virag Kothari added a comment -

          That calls getHTable()

          Show
          Virag Kothari added a comment - That calls getHTable()
          Hide
          Jimmy Xiang added a comment -

          How about getMetaHTable(final HConnection hConnection) (branch master)?

          Show
          Jimmy Xiang added a comment - How about getMetaHTable(final HConnection hConnection) (branch master)?
          Hide
          Virag Kothari added a comment -

          Probably we can use some methods in MetaTableAccessor?

          MetaTableAccessor does have a method getHTable(), but it uses a deprecated way of creating HTable. It should use connection.getHTable() so that a pool is managed by HConnection itself. But I think that can be done separately in other JIRA. Thoughts Jimmy Xiang?

          Show
          Virag Kothari added a comment - Probably we can use some methods in MetaTableAccessor? MetaTableAccessor does have a method getHTable(), but it uses a deprecated way of creating HTable. It should use connection.getHTable() so that a pool is managed by HConnection itself. But I think that can be done separately in other JIRA. Thoughts Jimmy Xiang ?
          Hide
          Virag Kothari added a comment -

          With threadlocal, how is the HTable closed?

          Good question. If we were to close it, we would need to store references to it which is expensive. So, instead if we skip the close it should be fine because the way htable is created, it wont shutdown the pool or close the connection (Both cleanupPoolOnClose and cleanupConnectionOnClose is false). So the only thing it would do is flushCommits but put() will take care of that as autoflush is true by default.

          With more connections, we can achieve some parallelism at the network/RPC layer.

          Yeah, more TCP connections are helping. I have seen the write throughput on meta region increase to around 450K/min from around 380k/min (~15%) when trying to assign 1M regions. Tweaking this configuration would help us when we to scale even further.

          Show
          Virag Kothari added a comment - With threadlocal, how is the HTable closed? Good question. If we were to close it, we would need to store references to it which is expensive. So, instead if we skip the close it should be fine because the way htable is created, it wont shutdown the pool or close the connection (Both cleanupPoolOnClose and cleanupConnectionOnClose is false). So the only thing it would do is flushCommits but put() will take care of that as autoflush is true by default. With more connections, we can achieve some parallelism at the network/RPC layer. Yeah, more TCP connections are helping. I have seen the write throughput on meta region increase to around 450K/min from around 380k/min (~15%) when trying to assign 1M regions. Tweaking this configuration would help us when we to scale even further.
          Hide
          Jimmy Xiang added a comment -

          With threadlocal, how is the HTable closed?

          The purpose of these HConnections is just to talk to the meta region. They share the same network connection, access the same region on the same region server. With more connections, we can achieve some parallelism at the network/RPC layer. It is another configuration, isn't it? Personally, I prefer your initial approach: share the HConnection from the server.

          Show
          Jimmy Xiang added a comment - With threadlocal, how is the HTable closed? The purpose of these HConnections is just to talk to the meta region. They share the same network connection, access the same region on the same region server. With more connections, we can achieve some parallelism at the network/RPC layer. It is another configuration, isn't it? Personally, I prefer your initial approach: share the HConnection from the server.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12661597/HBASE-11610.patch
          against trunk revision .
          ATTACHMENT ID: 12661597

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:

          -1 core zombie tests. There are 3 zombie test(s): at org.apache.hadoop.hbase.regionserver.TestHRegion.testWritesWhileGetting(TestHRegion.java:3808)
          at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScanBase.testScan(TestTableInputFormatScanBase.java:238)
          at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScan2.testScanOPPToEmpty(TestTableInputFormatScan2.java:70)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//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/12661597/HBASE-11610.patch against trunk revision . ATTACHMENT ID: 12661597 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: -1 core zombie tests . There are 3 zombie test(s): at org.apache.hadoop.hbase.regionserver.TestHRegion.testWritesWhileGetting(TestHRegion.java:3808) at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScanBase.testScan(TestTableInputFormatScanBase.java:238) at org.apache.hadoop.hbase.mapreduce.TestTableInputFormatScan2.testScanOPPToEmpty(TestTableInputFormatScan2.java:70) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10421//console This message is automatically generated.
          Hide
          Virag Kothari added a comment -

          Can we share one HConnection instead. We can create a HTable instance when needed. As long as we share the same HConnection, the performance should be ok.

          This is what I did initially. I had one HConnection and was creating htable instance for each put. But with more HConnections (tried with 5,10 and 15), the write throughput on meta regionserver increased and it brought the assignment time down by around 2 mins. The no of HConnection is configurable. By default, it is set to 1 as for small no of regions that is enough. Also, using threadlocal so we create only one htable instance for each thread accessing RegionStateStore.

          Show
          Virag Kothari added a comment - Can we share one HConnection instead. We can create a HTable instance when needed. As long as we share the same HConnection, the performance should be ok. This is what I did initially. I had one HConnection and was creating htable instance for each put. But with more HConnections (tried with 5,10 and 15), the write throughput on meta regionserver increased and it brought the assignment time down by around 2 mins. The no of HConnection is configurable. By default, it is set to 1 as for small no of regions that is enough. Also, using threadlocal so we create only one htable instance for each thread accessing RegionStateStore.
          Hide
          Jimmy Xiang added a comment -

          The performance number looks great. Can we share one HConnection instead. We can create a HTable instance when needed. As long as we share the same HConnection, the performance should be ok. Probably we can use some methods in MetaTableAccessor?

          Show
          Jimmy Xiang added a comment - The performance number looks great. Can we share one HConnection instead. We can create a HTable instance when needed. As long as we share the same HConnection, the performance should be ok. Probably we can use some methods in MetaTableAccessor?
          Hide
          Virag Kothari added a comment -

          Without patch, it took around 1hr 20 mins to assign 1M region. With patch, it takes around 12 mins. This is on 0.98

          Show
          Virag Kothari added a comment - Without patch, it took around 1hr 20 mins to assign 1M region. With patch, it takes around 12 mins. This is on 0.98
          Hide
          Virag Kothari added a comment -

          I was working on this and saw quite a bit of improvement when I used connection.getHTable() (uses a pool internally) to get a meta table instance. Will provide an update soon.

          Show
          Virag Kothari added a comment - I was working on this and saw quite a bit of improvement when I used connection.getHTable() (uses a pool internally) to get a meta table instance. Will provide an update soon.

            People

            • Assignee:
              Virag Kothari
              Reporter:
              Jimmy Xiang
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development