Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16067 )
Change subject: KUDU-3148: [test] Add Java client metrics ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java: http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java@39 PS5, Line 39: consider > nit: considered Done http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java@73 PS5, Line 73: * Enable or disable metric tracking. > It's probably worth noting that toggling this throws away existing metrics. Done http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java@76 PS5, Line 76: public static synchronized void setEnabled(boolean enable) { > Consider adding some tests for disabled metrics too, especially since it's Will do. It is public, though the interface is `@InterfaceAudience.Private` for now. http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java@97 PS5, Line 97: createdDisabledRegistry > nit: is there an extra "d" in the method name, i.e. should it be "createDis Done http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/KuduMetrics.java@112 PS5, Line 112: tags Must > nit: lower case Done http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java File java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java: http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@455 PS1, Line 455: ivate static String[] rpcTags(final AsyncKuduClient client, : final Connection connection, : final KuduRpc<?> rpc) { : return new String[] { : KuduMetrics.SERVICE_NAME_TAG, rpc.ser > My concern was less about an extra registering and more about the extra bui The construction of the builder object in java should be super low overhead and efficient, especially because the object is short lived. Java GC is especially good at small short-lived objects, especially these days. I think escape analysis would prevent any garbage/allocation as well. I am not sure we can avoid it given the tags are dynamic per call. http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java File java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java: http://gerrit.cloudera.org:8080/#/c/16067/5/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@122 PS5, Line 122: if (rpc.attempt == 1) { > Shouldn't we have a code path here that avoids any overhead of metrics? Eve Given we would like to have metrics on by default at some point I think it's okay to skip any sort of "is enabled" check. This code path is also super low overhead relative to RPCs. http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java: http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@495 PS1, Line 495: > OK, though it'd be nice to test the basics of client filtering when there a Done -- To view, visit http://gerrit.cloudera.org:8080/16067 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5c63835dd717c2c1e1dca06ed5dea3c2cadcd018 Gerrit-Change-Number: 16067 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 13 Jun 2020 13:16:49 +0000 Gerrit-HasComments: Yes
