Jean-Daniel Cryans has posted comments on this change. Change subject: Add statistics in java client. ......................................................................
Patch Set 1: (25 comments) http://gerrit.cloudera.org:8080/#/c/2858/1//COMMIT_MSG Commit Message: Line 9: The Statistics object will store information such as rpc count, Please add unit tests. 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 2150: * */ nit: this should be on two lines, see our the other javadoc is. Line 2159: public static final class Statistics { Why inner class? Also you need the InterfaceAudience and InterfaceStability annotations else this will not appear in the public javadoc. Line 2161: String, Statistics.TabletStatistics nit: since Java 7 you don't need to re-specify the template, you can remove everything I highlighted. Line 2164: * Increment the bytes written in the statistics nit: end your descriptions with a period (and fix all the others below). 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 being used by anyone. Line 2209: return new ArrayList<>(stsMap.keySet()); Use Collections.unmodifiableList() instead. Line 2291: Missing javadoc on a public method. Line 2302: ( nit: add a blank space here and for the other if statement in this method. Line 2313: { nit: missing blank space here an in the constructor. Line 2314: private AtomicLong bytesWritten = new AtomicLong(); Those can all be final. Line 2328: return "Table " + tableName + " tablet " + tabletId + ":" + Using a string builder would be cleaner. Line 2334: public void reset() { This is a public method in a private class and it's never called, remove? 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 67: . nit: don't put a period if there's only one sentence in @param or @return. Line 146: ( nit: missing blank space here, before the bracket, and for the other if and for statements in this method. Line 147: statistics.incrementRpcErrors(tableName, tabletId, 1); Shouldn't that also increment errors for all the operations? It's not clearly defined. Line 153: getRowError() != null call hasRowError Line 158: } catch (Throwable e) { What throws this? http://gerrit.cloudera.org:8080/#/c/2858/1/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java: Line 154: did nit: does Line 156: S nit: lower case, don't end with period. http://gerrit.cloudera.org:8080/#/c/2858/1/java/kudu-client/src/main/java/org/kududb/client/Operation.java File java/kudu-client/src/main/java/org/kududb/client/Operation.java: Line 189: ( nit: usual nit about missing blank spaces. Line 189: getRowError() != null call hasRowError Line 191: recored nit: recorded Line 195: } catch (Throwable e) { Same questions as in Batch. 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? -- 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-HasComments: Yes
