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 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/16067/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16067/1//COMMIT_MSG@20 PS1, Line 20: > nit: improvements Done http://gerrit.cloudera.org:8080/#/c/16067/1//COMMIT_MSG@24 PS1, Line 24: work (effectively writing something like micrometer). Outside > nit: implementation 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@453 PS1, Line 453: } > nit: spacing here and below Done 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 > Does this do any work if requestCounter() has already been called? Or is it Because the tags here are dynamic based on the local context, this call needs to lookup the meter based on the name + tags. If the meter already exists it won't do any other work beyond the hash lookup. When possible we should store meter instances in fields to avoid a lookup on each use. 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: > Can you also add a test that leverages multiple clients, to leverage the cl I added a filter based on the client id to make sure there is no outside noise. A tests for KUDU-1802 will likely use multiple clients. http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@518 PS1, Line 518: assertEquals(3, afterRequests - beforeRequests); > nit: maybe on failure we should print the metrics? I can log the proactively. http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@525 PS1, Line 525: validateSingleRpcSe > nit: maybe rename this so it's clear that the expected number of RPCs is on 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: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Jun 2020 16:07:56 +0000 Gerrit-HasComments: Yes
