HBase
  1. HBase
  2. HBASE-6870

HTable#coprocessorExec always scan the whole table

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.94.1, 0.95.0, 0.95.2
    • Fix Version/s: 0.98.0, 0.94.8, 0.95.1
    • Component/s: Coprocessors
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In current logic, HTable#coprocessorExec always scans the entire META table, loading it into memory and then filters the keys to return only those that fall in specified range. The version after the patch only scans the portions of meta that are in the specified key range, and returns them. Put simply – before we did a load-all-then-filter; afterwards we only-scan-what-is-needed.

      The former has low efficiency and greatly impacts the Regionserver carrying .META. when there are many coprocessorExec requests.

      1. HBASE-6870.patch
        6 kB
        chunhui shen
      2. HBASE-6870v2.patch
        5 kB
        chunhui shen
      3. HBASE-6870v3.patch
        6 kB
        chunhui shen
      4. HBASE-6870-testPerformance.patch
        3 kB
        chunhui shen
      5. 6870-v4.txt
        7 kB
        Ted Yu
      6. hbase-6870v5.patch
        7 kB
        chunhui shen
      7. hbase-6870v6.patch
        4 kB
        chunhui shen
      8. hbase-0.94-6870v6.patch
        4 kB
        chunhui shen
      9. hbase-0.95-6870v6.patch
        4 kB
        chunhui shen

        Issue Links

          Activity

          Hide
          Jonathan Hsieh added a comment -

          updated the description to make it easier to understand

          Show
          Jonathan Hsieh added a comment - updated the description to make it easier to understand
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #141 (See https://builds.apache.org/job/HBase-0.94-security/141/)
          HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470374)

          Result = FAILURE
          zjushch :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #141 (See https://builds.apache.org/job/HBase-0.94-security/141/ ) HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470374) Result = FAILURE zjushch : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #964 (See https://builds.apache.org/job/HBase-0.94/964/)
          HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470374)

          Result = SUCCESS
          zjushch :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #964 (See https://builds.apache.org/job/HBase-0.94/964/ ) HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470374) Result = SUCCESS zjushch : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #506 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/506/)
          HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470376)

          Result = FAILURE
          zjushch :
          Files :

          • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #506 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/506/ ) HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470376) Result = FAILURE zjushch : Files : /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Hide
          Hudson added a comment -

          Integrated in hbase-0.95-on-hadoop2 #76 (See https://builds.apache.org/job/hbase-0.95-on-hadoop2/76/)
          HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470375)

          Result = FAILURE
          zjushch :
          Files :

          • /hbase/branches/0.95/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Show
          Hudson added a comment - Integrated in hbase-0.95-on-hadoop2 #76 (See https://builds.apache.org/job/hbase-0.95-on-hadoop2/76/ ) HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470375) Result = FAILURE zjushch : Files : /hbase/branches/0.95/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Hide
          Hudson added a comment -

          Integrated in hbase-0.95 #156 (See https://builds.apache.org/job/hbase-0.95/156/)
          HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470375)

          Result = FAILURE
          zjushch :
          Files :

          • /hbase/branches/0.95/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Show
          Hudson added a comment - Integrated in hbase-0.95 #156 (See https://builds.apache.org/job/hbase-0.95/156/ ) HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470375) Result = FAILURE zjushch : Files : /hbase/branches/0.95/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #4073 (See https://builds.apache.org/job/HBase-TRUNK/4073/)
          HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470376)

          Result = SUCCESS
          zjushch :
          Files :

          • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #4073 (See https://builds.apache.org/job/HBase-TRUNK/4073/ ) HBASE-6870 HTable#coprocessorExec always scan the whole table (Revision 1470376) Result = SUCCESS zjushch : Files : /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HTable.java
          Hide
          Ted Yu added a comment -

          0.94.7 RC has potential to pass voting.

          Let's put 0.94.8 as Fix Version.

          Show
          Ted Yu added a comment - 0.94.7 RC has potential to pass voting. Let's put 0.94.8 as Fix Version.
          Hide
          chunhui shen added a comment -

          Committed to trunk, 0.95 and 0.94.

          Thanks for the review, Ted, Nicolas

          Show
          chunhui shen added a comment - Committed to trunk, 0.95 and 0.94. Thanks for the review, Ted, Nicolas
          Hide
          Ted Yu added a comment -

          +1 from me as well.

          Show
          Ted Yu added a comment - +1 from me as well.
          Hide
          Nicolas Liochon added a comment -

          +1 for v6!

          Show
          Nicolas Liochon added a comment - +1 for v6!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12579669/hbase-6870v6.patch
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

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

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

          +1 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 passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//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/12579669/hbase-6870v6.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/5371//console This message is automatically generated.
          Hide
          chunhui shen added a comment -

          Nicolas Liochon
          Thanks for the suggestion.
          Addressing these comments in patch v6.
          Now using a Pair of List instead of Map.

          Show
          chunhui shen added a comment - Nicolas Liochon Thanks for the suggestion. Addressing these comments in patch v6. Now using a Pair of List instead of Map.
          Hide
          Nicolas Liochon added a comment -

          Fascinating improvement I would say

          Some comments:

          • You've reorganized the imports, that will create conflicts when people will merge their patches ;-]
          • There is a parameter, but it's never used useCache with a value set to false. Wouldn't be better to remove it?
          • Just a question: why a LinkedHashMap?
          • The method should return the interface (Map) but not the real type.
          • Using the Map with a byte[] is brittle imho.
          • Does getKeysToRegionsInRange needs to be public?
          Show
          Nicolas Liochon added a comment - Fascinating improvement I would say Some comments: You've reorganized the imports, that will create conflicts when people will merge their patches ;-] There is a parameter, but it's never used useCache with a value set to false. Wouldn't be better to remove it? Just a question: why a LinkedHashMap? The method should return the interface (Map) but not the real type. Using the Map with a byte[] is brittle imho. Does getKeysToRegionsInRange needs to be public?
          Hide
          chunhui shen added a comment -

          About the performance:
          Total 256 regions in the table
          Concurrent 170 thread
          Each thread do AggregationClient#rowCount for one region

          Before the patch:
          Total 5s+

          After the patch:
          Total 200ms+

          Show
          chunhui shen added a comment - About the performance: Total 256 regions in the table Concurrent 170 thread Each thread do AggregationClient#rowCount for one region Before the patch: Total 5s+ After the patch: Total 200ms+
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12577422/hbase-6870v5.patch
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

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

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

          +1 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.zookeeper.lock.TestZKInterProcessReadWriteLock

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//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/12577422/hbase-6870v5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 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.zookeeper.lock.TestZKInterProcessReadWriteLock Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/5164//console This message is automatically generated.
          Hide
          chunhui shen added a comment -

          When useCache is true, what caching mechanism would getKeysToRegionsInRange() use ?

          We will use the cached region locations in HConnectionImplementation

          Show
          chunhui shen added a comment - When useCache is true, what caching mechanism would getKeysToRegionsInRange() use ? We will use the cached region locations in HConnectionImplementation
          Hide
          Ted Yu added a comment -
          +    if (useCache) {
          +      return new ArrayList<byte[]>(getKeysToRegionsInRange(start, end, true)
          +          .keySet());
          

          When useCache is true, what caching mechanism would getKeysToRegionsInRange() use ?

          Show
          Ted Yu added a comment - + if (useCache) { + return new ArrayList< byte []>(getKeysToRegionsInRange(start, end, true ) + .keySet()); When useCache is true, what caching mechanism would getKeysToRegionsInRange() use ?
          Hide
          chunhui shen added a comment -

          Path v5 fixes the warnings

          Show
          chunhui shen added a comment - Path v5 fixes the warnings
          Hide
          chunhui shen added a comment -

          How come useCache being true gets translated into includeEndKey being true ?

          The useCache flag is not related with includeEndKey, includeEndKey is always true in HTable#coprocessorService.

          I wonder if we should provide a mechanism for coprocessors to specify whether fresh .META. scan is wanted.

          Making a useCache flag is used for performance comparision in my initial purpose, I think there's no necessary to provide a choice for fresh .META. scan since it is still able to get a invalid region location from the .META.

          Show
          chunhui shen added a comment - How come useCache being true gets translated into includeEndKey being true ? The useCache flag is not related with includeEndKey, includeEndKey is always true in HTable#coprocessorService. I wonder if we should provide a mechanism for coprocessors to specify whether fresh .META. scan is wanted. Making a useCache flag is used for performance comparision in my initial purpose, I think there's no necessary to provide a choice for fresh .META. scan since it is still able to get a invalid region location from the .META.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12577014/6870-v4.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

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

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

          -1 lineLengths. The patch introduces lines longer than 100

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

          +1 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//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/12577014/6870-v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 3 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/5136//console This message is automatically generated.
          Hide
          Ted Yu added a comment -
          +  public LinkedHashMap<byte[], HRegionLocation> getKeysToRegionsInRange(
          +      final byte[] startKey, final byte[] endKey, final boolean includeEndKey)
          

          Later we have:

          +    if (useCache) {
          +      return new ArrayList<byte[]>(getKeysToRegionsInRange(start, end, true)
          +          .keySet());
          

          How come useCache being true gets translated into includeEndKey being true ?

          I wonder if we should provide a mechanism for coprocessors to specify whether fresh .META. scan is wanted.

          Show
          Ted Yu added a comment - + public LinkedHashMap< byte [], HRegionLocation> getKeysToRegionsInRange( + final byte [] startKey, final byte [] endKey, final boolean includeEndKey) Later we have: + if (useCache) { + return new ArrayList< byte []>(getKeysToRegionsInRange(start, end, true ) + .keySet()); How come useCache being true gets translated into includeEndKey being true ? I wonder if we should provide a mechanism for coprocessors to specify whether fresh .META. scan is wanted.
          Hide
          Ted Yu added a comment -

          Patch v4 is rebased on trunk.

          Show
          Ted Yu added a comment - Patch v4 is rebased on trunk.
          Hide
          Lars Hofhansl added a comment -

          Seems like a fix we'd want in 0.94 as well.

          Show
          Lars Hofhansl added a comment - Seems like a fix we'd want in 0.94 as well.
          Hide
          chunhui shen added a comment - - edited

          Current trunk still exists this problem:
          HTable#coprocessorService -> getStartKeysInRange -> getStartEndKeys -> getRegionLocations -> MetaScanner.allTableRegions(getConfiguration(), getTableName(), false)

          Scanning the whole table regions for each coprocessorService...

          I will update the patch after the vacation.

          Thanks for reviving, I have already forgot this one...

          Show
          chunhui shen added a comment - - edited Current trunk still exists this problem: HTable#coprocessorService -> getStartKeysInRange -> getStartEndKeys -> getRegionLocations -> MetaScanner.allTableRegions(getConfiguration(), getTableName(), false) Scanning the whole table regions for each coprocessorService... I will update the patch after the vacation. Thanks for reviving, I have already forgot this one...
          Hide
          Nicolas Liochon added a comment -

          I'm bumping the priority a little, because imho it means that anything that happen on .meta. will have a strong impact for anyone using coprocessors. I can help if needed of course.

          Show
          Nicolas Liochon added a comment - I'm bumping the priority a little, because imho it means that anything that happen on .meta. will have a strong impact for anyone using coprocessors. I can help if needed of course.
          Hide
          Gary Helmling added a comment -

          Ted Yu Yes, I'm looking through the last patch to get up to speed.

          Show
          Gary Helmling added a comment - Ted Yu Yes, I'm looking through the last patch to get up to speed.
          Hide
          Ted Yu added a comment -

          Gary Helmling:
          Do you think this issue should be revived ?

          Show
          Ted Yu added a comment - Gary Helmling : Do you think this issue should be revived ?
          Hide
          chunhui shen added a comment -

          Himanshu Vashishtha
          These two if statements is not made by this patch, so I just keep the previous.

          public LinkedHashMap<byte[], HRegionLocation> getKeysToRegionsInRange(
          

          Yes, it could be private.

          Thanks for the review.

          I will rework patch with other comments later

          Show
          chunhui shen added a comment - Himanshu Vashishtha These two if statements is not made by this patch, so I just keep the previous. public LinkedHashMap< byte [], HRegionLocation> getKeysToRegionsInRange( Yes, it could be private. Thanks for the review. I will rework patch with other comments later
          Hide
          Himanshu Vashishtha added a comment -

          Looked at the patch:

          Can you make the these two if statements in-line

          +        if (Bytes.compareTo(start, startKeys[i]) >= 0) {
          +          if (Bytes.equals(endKeys[i], HConstants.EMPTY_END_ROW)
          +              || Bytes.compareTo(start, endKeys[i]) < 0) {
          +            rangeKeys.add(start);
          +          }
          

          Can it be private?

          +  public LinkedHashMap<byte[], HRegionLocation> getKeysToRegionsInRange(
          

          Re: Andrew's concern regarding cache use: 6877 will take care of region move too? cache may become stale for reasons other than splits too. Will look at 6877.

          Show
          Himanshu Vashishtha added a comment - Looked at the patch: Can you make the these two if statements in-line + if (Bytes.compareTo(start, startKeys[i]) >= 0) { + if (Bytes.equals(endKeys[i], HConstants.EMPTY_END_ROW) + || Bytes.compareTo(start, endKeys[i]) < 0) { + rangeKeys.add(start); + } Can it be private? + public LinkedHashMap< byte [], HRegionLocation> getKeysToRegionsInRange( Re: Andrew's concern regarding cache use: 6877 will take care of region move too? cache may become stale for reasons other than splits too. Will look at 6877.
          Hide
          Andrew Purtell added a comment -

          Thanks chunhui shen.

          Show
          Andrew Purtell added a comment - Thanks chunhui shen .
          Hide
          chunhui shen added a comment -

          Coprocessor exec result is incorrect if cached region location is wrong HBASE-6877

          Show
          chunhui shen added a comment - Coprocessor exec result is incorrect if cached region location is wrong HBASE-6877
          Hide
          chunhui shen added a comment -

          Run the testPerformance on my local PC:

          Before:##Finished testAggregatePerformance, took time:7963ms ##

          After:##Finished testAggregatePerformance, took time:672ms ##

          In the test, I only create 64 regions for the table, if the region is much more, the result difference will more bigger

          Show
          chunhui shen added a comment - Run the testPerformance on my local PC: Before:##Finished testAggregatePerformance, took time:7963ms ## After:##Finished testAggregatePerformance, took time:672ms ## In the test, I only create 64 regions for the table, if the region is much more, the result difference will more bigger
          Hide
          chunhui shen added a comment -

          @Andrew

          First, we will also use cached location without this patch, see the details through
          HConnectionImplementation#processExecs->ExecRPCInvoker#invoke

          Second, if cached location is wrong, processExecs also will retry , also see the details in the ExecRPCInvoker#invoke

          Third, if one parent region is split when processExecs, current logic will cause the result incorrect, I will upload the patch to fix this bug after test in our environment

          At last, this patch is mainly in order to increase the performance for processExecs, I could provide the performance test later.

          Show
          chunhui shen added a comment - @Andrew First, we will also use cached location without this patch, see the details through HConnectionImplementation#processExecs->ExecRPCInvoker#invoke Second, if cached location is wrong, processExecs also will retry , also see the details in the ExecRPCInvoker#invoke Third, if one parent region is split when processExecs, current logic will cause the result incorrect, I will upload the patch to fix this bug after test in our environment At last, this patch is mainly in order to increase the performance for processExecs, I could provide the performance test later.
          Hide
          Andrew Purtell added a comment -

          A coprocessorExec to an incorrect (cached) location will cause an exception to be sent back to the client, processExecs does not do transparent relocation. I don't think we want this. The patch looks ok without the cache.

          Show
          Andrew Purtell added a comment - A coprocessorExec to an incorrect (cached) location will cause an exception to be sent back to the client, processExecs does not do transparent relocation. I don't think we want this. The patch looks ok without the cache.
          Hide
          chunhui shen added a comment -

          PatchV2's directory is wrong,

          Upload new patch v3

          Show
          chunhui shen added a comment - PatchV2's directory is wrong, Upload new patch v3
          Hide
          chunhui shen added a comment -

          We have took a simple test:
          Concurrent 170 thread doing coprocessorExec
          Each coprocessorExec do a scan for one region
          Total 256 regions in the table

          Before the patch:
          Total 5s+

          After the patch:
          Total 200ms+

          Show
          chunhui shen added a comment - We have took a simple test: Concurrent 170 thread doing coprocessorExec Each coprocessorExec do a scan for one region Total 256 regions in the table Before the patch: Total 5s+ After the patch: Total 200ms+
          Hide
          chunhui shen added a comment -

          Instead of scanning .META. for whole table,We could use region location cache

          Show
          chunhui shen added a comment - Instead of scanning .META. for whole table,We could use region location cache

            People

            • Assignee:
              chunhui shen
              Reporter:
              chunhui shen
            • Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development