d-c-manning commented on code in PR #5023:
URL: https://github.com/apache/hbase/pull/5023#discussion_r1113478340
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -638,16 +641,23 @@ private void shutdown() {
}
/** Report RPC context to metrics system. */
- public void updateRpc(MethodDescriptor method, Message param, CallStats
stats, boolean failed) {
+ public void updateRpc(MethodDescriptor method, Message param, CallStats
stats, Throwable e) {
int callsPerServer = stats.getConcurrentCallsPerServer();
if (callsPerServer > 0) {
concurrentCallsPerServerHist.update(callsPerServer);
}
// Update the counter that tracks RPCs by type.
final String methodName = method.getService().getName() + "_" +
method.getName();
getMetric(CNT_BASE + methodName, rpcCounters, counterFactory).inc();
- if (failed) {
+ if (e != null) {
getMetric(FAILURE_CNT_BASE + methodName, rpcCounters,
counterFactory).inc();
+ if (e instanceof RemoteWithExtrasException) {
+ getMetric(REMOTE_EXCEPTION_CNT_BASE + ((RemoteWithExtrasException)
e).getClassName(),
+ rpcCounters, counterFactory).inc();
Review Comment:
It looks like we're not using any capabilities beyond the base class of
`RemoteException` - should we use that instead?
```suggestion
if (e instanceof RemoteException) {
getMetric(REMOTE_EXCEPTION_CNT_BASE + ((RemoteException)
e).getClassName(),
rpcCounters, counterFactory).inc();
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -638,16 +641,23 @@ private void shutdown() {
}
/** Report RPC context to metrics system. */
- public void updateRpc(MethodDescriptor method, Message param, CallStats
stats, boolean failed) {
+ public void updateRpc(MethodDescriptor method, Message param, CallStats
stats, Throwable e) {
int callsPerServer = stats.getConcurrentCallsPerServer();
if (callsPerServer > 0) {
concurrentCallsPerServerHist.update(callsPerServer);
}
// Update the counter that tracks RPCs by type.
final String methodName = method.getService().getName() + "_" +
method.getName();
getMetric(CNT_BASE + methodName, rpcCounters, counterFactory).inc();
- if (failed) {
+ if (e != null) {
getMetric(FAILURE_CNT_BASE + methodName, rpcCounters,
counterFactory).inc();
+ if (e instanceof RemoteWithExtrasException) {
+ getMetric(REMOTE_EXCEPTION_CNT_BASE + ((RemoteWithExtrasException)
e).getClassName(),
Review Comment:
`getClassName` will add a fully qualified package name into the metric name,
right? The metric names will become unwieldy since we would have
`rpcRemoteExceptions_org.apache.hadoop.hbase.CallQueueTooBigException`. If we
stick with `RemoteWithExtrasException` we could use
`RemoteWithExtrasException#unwrapRemoteException().getSimpleName()` but that
seems heavy. If we can assume that `getClassName` will always return a
"standard" Exception class, we could just use string manipulation to get the
String after the last `.`?
##########
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java:
##########
@@ -150,51 +152,71 @@ public void testStaticMetrics() throws IOException {
for (int i = 0; i < loop; i++) {
METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Get"),
- GetRequest.getDefaultInstance(), MetricsConnection.newCallStats(),
false);
+ GetRequest.getDefaultInstance(), MetricsConnection.newCallStats(),
null);
METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Scan"),
- ScanRequest.getDefaultInstance(), MetricsConnection.newCallStats(),
false);
+ ScanRequest.getDefaultInstance(), MetricsConnection.newCallStats(),
+ new RemoteWithExtrasException("IOException", null, false, false));
Review Comment:
similar to other comment - is this possible, or would it always be
```suggestion
new RemoteWithExtrasException("java.io.IOException", null, false,
false));
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java:
##########
@@ -638,16 +641,23 @@ private void shutdown() {
}
/** Report RPC context to metrics system. */
- public void updateRpc(MethodDescriptor method, Message param, CallStats
stats, boolean failed) {
+ public void updateRpc(MethodDescriptor method, Message param, CallStats
stats, Throwable e) {
int callsPerServer = stats.getConcurrentCallsPerServer();
if (callsPerServer > 0) {
concurrentCallsPerServerHist.update(callsPerServer);
}
// Update the counter that tracks RPCs by type.
final String methodName = method.getService().getName() + "_" +
method.getName();
getMetric(CNT_BASE + methodName, rpcCounters, counterFactory).inc();
- if (failed) {
+ if (e != null) {
getMetric(FAILURE_CNT_BASE + methodName, rpcCounters,
counterFactory).inc();
+ if (e instanceof RemoteWithExtrasException) {
Review Comment:
```suggestion
getMetric(ALL_EXCEPTIONS, rpcCounters, counterFactory).inc();
if (e instanceof RemoteWithExtrasException) {
```
I would be inclined to have a summary counter for exceptions, similar to the
server-side metric in
https://github.com/apache/hbase/blob/22dbb7afc383b9b9a8678f2c9eca1dca31784615/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java#L100-L110
- it would make the total comparisons easier, and you don't have to know all
the exception types to sum them up.
--
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]
