Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16067 )
Change subject: WIP: [java] Add Java client metrics ...................................................................... Patch Set 1: (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: improvments nit: improvements http://gerrit.cloudera.org:8080/#/c/16067/1//COMMIT_MSG@24 PS1, Line 24: implimentation because it didn’t appear to be flexible nit: implementation 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: final Connection connection, nit: spacing here and below http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@455 PS1, Line 455: return Counter.builder(KuduMetrics.RPC_REQUESTS_METRIC) : .description("A count of the sent request RPCs") : .baseUnit("requests") : .tags(rpcTags(client, connection, rpc)) : .register(KuduMetrics.getRegistry()); Does this do any work if requestCounter() has already been called? Or is it just a lookup in the registry? Could we do that lookup up front if it's more than that, e.g. to avoid construction of the builder otherwise? 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: KuduTable table = client.createTable(testTableName, schema, createOptions); Can you also add a test that leverages multiple clients, to leverage the client ID tagging? I would have suggested having the metrics be a part of the client instance, rather than being a singleton, but perhaps doing so would make it trickier to map back to server traffic which encompasses metrics from all 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? http://gerrit.cloudera.org:8080/#/c/16067/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@525 PS1, Line 525: validateExpectedRpc nit: maybe rename this so it's clear that the expected number of RPCs is one. E.g. validateSingleRpcSent() or something -- 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: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 11 Jun 2020 16:51:05 +0000 Gerrit-HasComments: Yes
