[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-09-12 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1323080007


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionUtils.java:
##
@@ -196,13 +196,17 @@ static boolean isEmptyStopRow(byte[] row) {
 return Bytes.equals(row, EMPTY_END_ROW);
   }
 
-  static void resetController(HBaseRpcController controller, long timeoutNs, 
int priority) {
+  static void resetController(HBaseRpcController controller, long timeoutNs, 
int priority,
+TableName tableName) {
 controller.reset();
 if (timeoutNs >= 0) {
   controller.setCallTimeout(
 (int) Math.min(Integer.MAX_VALUE, 
TimeUnit.NANOSECONDS.toMillis(timeoutNs)));
 }
 controller.setPriority(priority);
+if (tableName != null) {
+  controller.setTableName(tableName);

Review Comment:
   @zhuyaogai I think you are right that its unlikely for the table name to 
change for the same RpcRetryingCaller. I'm just trying to ensure that we code 
defensively here, since who knows how the code will change in the future.
   
   That said, the contract seems to be that we always should reset the 
controller before using it. So I think you're good to just reset the table name 
there, and leave this code here as-is.



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-09-12 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1322934452


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionUtils.java:
##
@@ -196,13 +196,17 @@ static boolean isEmptyStopRow(byte[] row) {
 return Bytes.equals(row, EMPTY_END_ROW);
   }
 
-  static void resetController(HBaseRpcController controller, long timeoutNs, 
int priority) {
+  static void resetController(HBaseRpcController controller, long timeoutNs, 
int priority,
+TableName tableName) {
 controller.reset();
 if (timeoutNs >= 0) {
   controller.setCallTimeout(
 (int) Math.min(Integer.MAX_VALUE, 
TimeUnit.NANOSECONDS.toMillis(timeoutNs)));
 }
 controller.setPriority(priority);
+if (tableName != null) {
+  controller.setTableName(tableName);

Review Comment:
   sorry i just caught this -- do you think we should setTableName with a null 
value when passed in? This would indicate a request that has no table, but if 
we don't set to null then it might attribute the request to an old table name 
since controllers are re-used. Alternatively, controller.reset() should reset 
the table name.



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-09-11 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1321620558


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##
@@ -692,6 +711,13 @@ public void updateRpc(MethodDescriptor method, Message 
param, CallStats stats, T
 // this implementation is tied directly to protobuf implementation 
details. would be better
 // if we could dispatch based on something static, ie, request Message 
type.
 if (method.getService() == ClientService.getDescriptor()) {
+  if (
+"Get".equals(method.getName()) || "Mutate".equals(method.getName())
+  || "Scan".equals(method.getName()) || 
"Multi".equals(method.getName())
+  ) {
+updateTableMetric(methodName.toString(), tableName, stats, e);

Review Comment:
   do we still need this, or can we simply add the call to `updateTableMetric` 
in the appropriate switch state cases below? Just to avoid unnecessary 
complexity and equality checks



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-09-08 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1319809146


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncClientScanner.java:
##
@@ -172,6 +172,7 @@ private CompletableFuture 
callOpenScanner(HBaseRpcControlle
   }
   CompletableFuture future = new 
CompletableFuture<>();
   try {
+controller.setTableName(loc.getRegion().getTable());

Review Comment:
   I'm pretty sure this is unnecessary. This `callOpenScanner` ends up getting 
called via AsyncSingleRequestRpcRetryingCaller, which does `resetCallTimeout()` 
before calling the callable. So the change you made in that method should 
handle this case



##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRpcRetryingCaller.java:
##
@@ -121,7 +121,11 @@ protected final void resetCallTimeout() {
 } else {
   callTimeoutNs = rpcTimeoutNs;
 }
-resetController(controller, callTimeoutNs, priority);
+if (getTableName().isPresent()) {

Review Comment:
   could be simplified to one call, with `getTableName().orElse(null)`



##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##
@@ -753,6 +787,40 @@ public void updateRpc(MethodDescriptor method, Message 
param, CallStats stats, T
 updateRpcGeneric(methodName.toString(), stats);
   }
 
+  /** Report table rpc context to metrics system. */
+  private void updateTableMetric(String methodName, TableName tableName,
+HBaseProtos.RegionSpecifier regionSpecifier, Message param, CallStats 
stats, Throwable e) {
+if (tableMetricsEnabled) {
+  if (methodName != null) {
+String table;
+if (tableName == null || 
StringUtils.isEmpty(tableName.getNameAsString())) {
+  // Fallback to get table name from region specifier.

Review Comment:
   i think we can simplify this whole method (and above calls) now. I'm not 
sure we need to handle fallback, since all cases should have a TableName in the 
controller now. So we don't need to extract the region in updateRpc above, nor 
do we have to to parse the tablename from the region here.
   
   if we missed a spot, i think we'd consider that a bug and fix it.  maybe you 
could add end-to-end tests to ensure that each of the request types updates a 
table metric?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-08-19 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1299173064


##
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java:
##
@@ -346,10 +346,9 @@ public static RegionAction.Builder 
getRegionActionBuilderWithRegion(
   public static ScanRequest buildScanRequest(byte[] regionName, Scan scan, int 
numberOfRows,
 boolean closeScanner) throws IOException {
 ScanRequest.Builder builder = ScanRequest.newBuilder();
-RegionSpecifier region = 
buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName);
+builder.setRegion(buildRegionSpecifier(RegionSpecifierType.REGION_NAME, 
regionName));

Review Comment:
   Is there something else you’d need? I’d go down the path I described, which 
I feel pretty good about. Just duo often has nice ideas so I figured I’d ask, 
but he’s probably busy. 



##
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java:
##
@@ -346,10 +346,9 @@ public static RegionAction.Builder 
getRegionActionBuilderWithRegion(
   public static ScanRequest buildScanRequest(byte[] regionName, Scan scan, int 
numberOfRows,
 boolean closeScanner) throws IOException {
 ScanRequest.Builder builder = ScanRequest.newBuilder();
-RegionSpecifier region = 
buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName);
+builder.setRegion(buildRegionSpecifier(RegionSpecifierType.REGION_NAME, 
regionName));

Review Comment:
   Is there something else you’d need? I’d go down the path I described, which 
I feel pretty good about. Just duo often has nice ideas so I figured I’d ask, 
but he’s probably busy. 



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-08-14 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1293383761


##
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java:
##
@@ -346,10 +346,9 @@ public static RegionAction.Builder 
getRegionActionBuilderWithRegion(
   public static ScanRequest buildScanRequest(byte[] regionName, Scan scan, int 
numberOfRows,
 boolean closeScanner) throws IOException {
 ScanRequest.Builder builder = ScanRequest.newBuilder();
-RegionSpecifier region = 
buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName);
+builder.setRegion(buildRegionSpecifier(RegionSpecifierType.REGION_NAME, 
regionName));

Review Comment:
   Thanks for explaining.
   
   I'm not sure we should modify the RPC protocol just for the sake of these 
client side metrics. It might be better to use HBaseRpcController to shuttle 
the table name into the `onCallFinished` method (where `updateRpc` is called). 
As an example, you could modify AsyncScanSingleRegionRpcRetryingCaller.call 
method:
   
   ```diff
   - resetController(controller, callTimeoutNs, priority);
   + resetController(controller, callTimeoutNs, priority, 
loc.getRegion().getTable());
   ScanRequest req = RequestConverter.buildScanRequest(scannerId, 
scan.getCaching(), false,
 nextCallSeq, scan.isScanMetricsEnabled(), false, scan.getLimit());
   final Context context = Context.current();
   stub.scan(controller, req, resp -> {
 try (Scope ignored = context.makeCurrent()) {
   onComplete(controller, resp);
 }
   });
   ```
   
   This requires a bit more changes, but at least it keeps our RPC protocol 
unchanged.
   
   You'll need to add a setTableName and getTableName to HBaseRpcController and 
DelegatingHBaseRpcController. Then update that 
`ConnectionUtils.resetController` method (and callers), as well as 
RegionServerCallable once on branch-2.
   
   @Apache9 if you have time, do you agree with this advice?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5228: HBASE-27853 Add client side table metrics for rpc calls and request latency.

2023-08-11 Thread via GitHub 


bbeaudreault commented on code in PR #5228:
URL: https://github.com/apache/hbase/pull/5228#discussion_r1269558095


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##
@@ -732,6 +770,32 @@ public void updateRpc(MethodDescriptor method, Message 
param, CallStats stats, T
 updateRpcGeneric(methodName, stats);
   }
 
+  /** Report table rpc context to metrics system. */
+  private void updateTableMetric(String methodName, String tableName, 
CallStats stats,
+Throwable e) {
+if (
+  conf.getBoolean(CLIENT_SIDE_TABLE_METRICS_ENABLED_KEY, false) && 
methodName != null

Review Comment:
   can you do the conf.getBoolean call once in the MetricsConnection 
constructor? parsing conf can cause perf issues in hot codepaths, so its best 
to cache imo



##
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java:
##
@@ -346,10 +346,9 @@ public static RegionAction.Builder 
getRegionActionBuilderWithRegion(
   public static ScanRequest buildScanRequest(byte[] regionName, Scan scan, int 
numberOfRows,
 boolean closeScanner) throws IOException {
 ScanRequest.Builder builder = ScanRequest.newBuilder();
-RegionSpecifier region = 
buildRegionSpecifier(RegionSpecifierType.REGION_NAME, regionName);
+builder.setRegion(buildRegionSpecifier(RegionSpecifierType.REGION_NAME, 
regionName));

Review Comment:
   what is the reason for these changes in this class?



##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##
@@ -732,6 +770,32 @@ public void updateRpc(MethodDescriptor method, Message 
param, CallStats stats, T
 updateRpcGeneric(methodName, stats);
   }
 
+  /** Report table rpc context to metrics system. */
+  private void updateTableMetric(String methodName, String tableName, 
CallStats stats,
+Throwable e) {
+if (
+  conf.getBoolean(CLIENT_SIDE_TABLE_METRICS_ENABLED_KEY, false) && 
methodName != null
+&& tableName != null
+) {
+  String metricKey = methodName + "_" + tableName;
+  updateRpcGeneric(metricKey, stats);
+  if (e != null) {
+getMetric(FAILURE_CNT_BASE + metricKey, rpcCounters, 
counterFactory).inc();
+  }
+}
+  }
+
+  /** Parse table from region. */
+  private String parseTableName(HBaseProtos.RegionSpecifier regionSpecifier, 
Message param) {
+if (regionSpecifier == null || 
StringUtils.isEmpty(regionSpecifier.getValue().toStringUtf8())) {

Review Comment:
   can you move this into your updateTableMetrics method so that we don't have 
to do the parsing unless table metrics are enabled?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org