Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/16067 )
Change subject: KUDU-3148: [test] Add Java client metrics ...................................................................... Patch Set 5: (3 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 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 "createDisabledRegistry" not "createdDisabledRegistry"? 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? Even though, by default, the registry will be disabled, this code path, still adds some overhead of calling several getters, setters and a constructor. -- 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 <granthe...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Jun 2020 17:00:41 +0000 Gerrit-HasComments: Yes
