Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16067 )
Change subject: KUDU-3148: [test] Add Java client metrics ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2513 PS1, Line 2513: // 3. Release all other resources. Maybe add a test for this? 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@73 PS5, Line 73: * Enable or disable metric tracking. It's probably worth noting that toggling this throws away existing metrics. 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 public. 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 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 > Because the tags here are dynamic based on the local context, this call nee My concern was less about an extra registering and more about the extra building that we seem to be doing on every call. Are there APIs in micrometer to avoid that? 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: > I added a filter based on the client id to make sure there is no outside no OK, though it'd be nice to test the basics of client filtering when there are multiple clients to filter, given that's a part of this patch. -- 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: Fri, 12 Jun 2020 20:27:09 +0000 Gerrit-HasComments: Yes
