Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18489 )
Change subject: [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API ...................................................................... Patch Set 1: Code-Review+1 (7 comments) LGTM, just a few nits http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java File java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java@188 PS1, Line 188: style nit: the indent should be 4 spaces, IIRC http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java@129 PS1, Line 129: ResourceMetrics getWriteOpMetrics() { nit: does it make sense to add @Nullable annotation here? http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java File java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@35 PS1, Line 35: * A container for scanner resource metrics. : * <p> : * This class wraps a mapping from metric name to metric value for server-side : * metrics associated with a scanner. nit: could you please update this? The ResourceMetrics are now used in the write path as well with this patch. http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@87 PS1, Line 87: String key = entry.getKey(); nit: this variable is used only once in the increment() call below, so maybe inline the 'entry.getKey()' call instead of introducing a variable? http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@104 PS1, Line 104: its pb nit: PB http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@104 PS1, Line 104: not nit: not be http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@347 PS1, Line 347: public nit: maybe, change to private if it's not used anywhere else? -- To view, visit http://gerrit.cloudera.org:8080/18489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb Gerrit-Change-Number: 18489 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 05 May 2022 01:43:24 +0000 Gerrit-HasComments: Yes
