[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r333166635 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java ## @@ -89,18 +91,17 @@ */ long getResponseCellSize(); + long getNumsOfGet(); + + long incrementGetsNum(); + /** - * Add on the given amount to the retained cell size. - * - * This is not thread safe and not synchronized at all. If this is used by more than one thread - * then everything will break. Since this is called for every row synchronization would be too - * onerous. + * Method to account for the size of retained cells and retained data blocks. + * @param result result to add size. */ - void incrementResponseCellSize(long cellSize); - - long getResponseBlockSize(); Review comment: Cancelling my comment. The Interface is @Private This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an
option to enable multiget in parallel
URL: https://github.com/apache/hbase/pull/656#discussion_r333168029
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##
@@ -788,31 +791,28 @@ private Result increment(final HRegion region, final
OperationQuota quota,
List mutations = null;
long maxQuotaResultSize = Math.min(maxScannerResultSize,
quota.getReadAvailable());
IOException sizeIOE = null;
-Object lastBlock = null;
-ClientProtos.ResultOrException.Builder resultOrExceptionBuilder =
ResultOrException.newBuilder();
+List getCtxs = new ArrayList<>();
+if (cellsToReturn == null && isClientCellBlockSupport(context)) {
+ cellsToReturn = new ArrayList();
+}
+ResultOrException.Builder resultOrExceptionBuilder =
ResultOrException.newBuilder();
boolean hasResultOrException = false;
for (ClientProtos.Action action : actions.getActionList()) {
hasResultOrException = false;
resultOrExceptionBuilder.clear();
try {
Result r = null;
-
-if (context != null
-&& context.isRetryImmediatelySupported()
-&& (context.getResponseCellSize() > maxQuotaResultSize
- || context.getResponseBlockSize() +
context.getResponseExceptionSize()
- > maxQuotaResultSize)) {
-
+if (isRpcCallAboveQuota(context, maxQuotaResultSize, true)) {
Review comment:
Good change.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r333167497 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -972,6 +972,17 @@ /** Whether nonces are enabled; default is true. */ public static final String HBASE_RS_NONCES_ENABLED = "hbase.regionserver.nonces.enabled"; + /** Some configuration param about multiget in parallel */ + public static final String HBASE_RS_PARALLEL_GET_ENABLED = + "hbase.regionserver.parallel.get.enabled"; + public static final boolean DEFAULT_PARALLEL_GET_ENABLED = false; Review comment: Why not enable it by default? What would be the downside? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r328714184 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -969,6 +969,11 @@ public static final String HBASE_REGION_SPLIT_POLICY_KEY = "hbase.regionserver.region.split.policy"; + public static final String RS_PARALLEL_GET_ENABLED = "hbase.server.parallel.get.enabled"; + public static final String RS_PARALLEL_GET_THREADS = "hbase.server.parallel.get.threads"; Review comment: Stats on executors for general RS sounds good. Need feedback to figure how to better make use of the feature. Add to release note stuff like '20 threads by default'. I'd be +1 for having it on by default but 20 seems like a high number. 5 threads by default? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r327936808 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -969,6 +969,11 @@ public static final String HBASE_REGION_SPLIT_POLICY_KEY = "hbase.regionserver.region.split.policy"; + public static final String RS_PARALLEL_GET_ENABLED = "hbase.server.parallel.get.enabled"; + public static final String RS_PARALLEL_GET_THREADS = "hbase.server.parallel.get.threads"; Review comment: How do I get feedback on how many threads I should run with? What do I look at? Is there a log message or a metric I can see? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an
option to enable multiget in parallel
URL: https://github.com/apache/hbase/pull/656#discussion_r327937888
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##
@@ -408,10 +411,10 @@ public void run() throws IOException {
* An RpcCallBack that creates a list of scanners that needs to perform
callBack operation on
* completion of multiGets.
*/
- static class RegionScannersCloseCallBack implements RpcCallback {
+ public static class RegionScannersCloseCallBack implements RpcCallback {
private final List scanners = new ArrayList<>();
-public void addScanner(RegionScanner scanner) {
+public synchronized void addScanner(RegionScanner scanner) {
Review comment:
Why we need this now?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r327937062 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java ## @@ -89,18 +91,17 @@ */ long getResponseCellSize(); + long getNumsOfGet(); Review comment: Just spell out Number ... getNumberOfGets This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r327936909 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -969,6 +969,11 @@ public static final String HBASE_REGION_SPLIT_POLICY_KEY = "hbase.regionserver.region.split.policy"; + public static final String RS_PARALLEL_GET_ENABLED = "hbase.server.parallel.get.enabled"; + public static final String RS_PARALLEL_GET_THREADS = "hbase.server.parallel.get.threads"; Review comment: How does this bloat up our thread count? Already the RegionServer has too many. How many threads will this add? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r327937217 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java ## @@ -89,18 +91,17 @@ */ long getResponseCellSize(); + long getNumsOfGet(); + + long incrementGetsNum(); + /** - * Add on the given amount to the retained cell size. - * - * This is not thread safe and not synchronized at all. If this is used by more than one thread - * then everything will break. Since this is called for every row synchronization would be too - * onerous. + * Method to account for the size of retained cells and retained data blocks. + * @param result result to add size. */ - void incrementResponseCellSize(long cellSize); - - long getResponseBlockSize(); Review comment: Did these get removed? Is that ok? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel URL: https://github.com/apache/hbase/pull/656#discussion_r327937101 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java ## @@ -89,18 +91,17 @@ */ long getResponseCellSize(); + long getNumsOfGet(); + + long incrementGetsNum(); Review comment: incrementGetsNumber This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #656: HBASE-23063 Add an option to enable multiget in parallel
saintstack commented on a change in pull request #656: HBASE-23063 Add an
option to enable multiget in parallel
URL: https://github.com/apache/hbase/pull/656#discussion_r327937450
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java
##
@@ -431,31 +436,42 @@ public boolean isClientCellBlockSupported() {
@Override
public long getResponseCellSize() {
-return responseCellSize;
+return responseCellSize.get();
}
@Override
- public void incrementResponseCellSize(long cellSize) {
Review comment:
Are these methods removed?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
