zhen.zhang has posted comments on this change. Change subject: Add statistics in java client. ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/2858/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 2167: public void incrementBytesWritten(String tableName, String tabletId, long newBytes) { > I don't think we want this to be public, else we open these methods to bein yes, you are right Line 2175: public void incrementWriteOps(String tableName, String tabletId, int count) { > maybe we could avoid all of these redundant methods by doing something like cool! I'll refactor the code. Line 2334: public void reset() { > This is a public method in a private class and it's never called, remove? OK http://gerrit.cloudera.org:8080/#/c/2858/1/java/kudu-client/src/main/java/org/kududb/client/Batch.java File java/kudu-client/src/main/java/org/kududb/client/Batch.java: Line 146: ( > nit: missing blank space here, before the bracket, and for the other if and Yes, thanks for the careful review. Will format the code. Line 147: statistics.incrementRpcErrors(tableName, tabletId, 1); > Shouldn't that also increment errors for all the operations? It's not clear Yes, I think we should also increment the opts_errors. At first I think opts_errors only stands for exceptions like "ALREADY_PRESENT", so I didn't count here. But maybe it's more reasonable to treat opts_errors as "opts_fails". Line 153: getRowError() != null > call hasRowError Yes Line 158: } catch (Throwable e) { > What throws this? It seems nobody throws this. Will remove this catch. http://gerrit.cloudera.org:8080/#/c/2858/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java: Line 35: import org.kududb.WireProtocol.RowOperationsPB; > What's with all those new imports? Imported by some old code. Will delete them. -- To view, visit http://gerrit.cloudera.org:8080/2858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic9f06bcae7afac69772e55e85a020a4fe90ae845 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: zhen.zhang <[email protected]> Gerrit-HasComments: Yes
