Apache9 commented on code in PR #5224:
URL: https://github.com/apache/hbase/pull/5224#discussion_r1199612767
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -652,7 +652,27 @@ public void updateRpc(MethodDescriptor method, Message
param, CallStats stats, T
concurrentCallsPerServerHist.update(callsPerServer);
}
// Update the counter that tracks RPCs by type.
- final String methodName = method.getService().getName() + "_" +
method.getName();
+ String methodName = method.getService().getName() + "_" + method.getName();
Review Comment:
Better use a StringBuilder here and finally build the methodName through
StringBuilder.toString?
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java:
##########
@@ -215,13 +234,13 @@ public void testStaticMetrics() throws IOException {
metricKey = "rpcLocalExceptions_CallTimeoutException";
counter = METRICS.getRpcCounters().get(metricKey);
metricVal = (counter != null) ? counter.getCount() : 0;
- assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal ==
loop);
+ assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal ==
loop * 2);
// total exception
metricKey = "rpcTotalExceptions";
counter = METRICS.getRpcCounters().get(metricKey);
metricVal = (counter != null) ? counter.getCount() : 0;
- assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal ==
loop * 2);
+ assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal ==
loop * 3);
Review Comment:
Ditto. Please change to use assertEquals
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java:
##########
@@ -178,7 +178,8 @@ public void testStaticMetrics() throws IOException {
MutateRequest.newBuilder()
.setMutation(ProtobufUtil.toMutation(MutationType.PUT, new
Put(foo))).setRegion(region)
.build(),
- MetricsConnection.newCallStats(), null);
+ MetricsConnection.newCallStats(),
+ new CallTimeoutException("test with CallTimeoutException"));
Review Comment:
Why here we need to change null to a CallTimeoutException?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -652,7 +652,27 @@ public void updateRpc(MethodDescriptor method, Message
param, CallStats stats, T
concurrentCallsPerServerHist.update(callsPerServer);
}
// Update the counter that tracks RPCs by type.
- final String methodName = method.getService().getName() + "_" +
method.getName();
+ String methodName = method.getService().getName() + "_" + method.getName();
+ // Distinguish mutate types.
+ if ("Mutate".equals(method.getName())) {
+ final MutationType type = ((MutateRequest)
param).getMutation().getMutateType();
+ switch (type) {
+ case APPEND:
+ methodName += "(Append)";
+ break;
+ case DELETE:
+ methodName += "(Delete)";
+ break;
+ case INCREMENT:
+ methodName += "(Increment)";
+ break;
+ case PUT:
+ methodName += "(Put)";
+ break;
+ default:
+ throw new RuntimeException("Unrecognized mutation type " + type);
Review Comment:
This is for metrics so usually we should not throw exception again,
appending `(Unknown)` or just appending nothing is enough?
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java:
##########
@@ -215,13 +234,13 @@ public void testStaticMetrics() throws IOException {
metricKey = "rpcLocalExceptions_CallTimeoutException";
counter = METRICS.getRpcCounters().get(metricKey);
metricVal = (counter != null) ? counter.getCount() : 0;
- assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal ==
loop);
+ assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal ==
loop * 2);
Review Comment:
Not your fault but better use assertEquals here.
--
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]
