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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
