Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1415. Add statistics in java client. ......................................................................
Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/2858/2/java/kudu-client/src/main/java/org/kududb/client/Batch.java File java/kudu-client/src/main/java/org/kududb/client/Batch.java: Line 158: statistics.incrementStatistic(tableName, tabletId, Statistic.BYTES_WRITTEN, getOperationBytesSize()); nit: we try to keep lines under 100 chars, please wrap. http://gerrit.cloudera.org:8080/#/c/2858/2/java/kudu-client/src/main/java/org/kududb/client/Statistics.java File java/kudu-client/src/main/java/org/kududb/client/Statistics.java: Line 32: * This class stores this client's statistics information, including the rpc count, This will need a lot more javadoc, this it will be part of the public API. Can a user just get the statistics object from the client once and always directly use it? Is it just for one session or the whole client? etc Line 38: ConcurrentHashMap<String, Statistics.TabletStatistics> stsMap = Make it private final, also it doesn't need line wrapping. Line 53: succeed nit: succeeded Line 57: succeed same nit Line 87: talbeName nit Line 115: ){ nit: missing blank space. Line 122: tablet id nit: the param's name is tabletId Line 125: ){ nit: missing blank space. http://gerrit.cloudera.org:8080/#/c/2858/2/java/kudu-client/src/test/java/org/kududb/client/TestStatistics.java File java/kudu-client/src/test/java/org/kududb/client/TestStatistics.java: Line 19: import static org.junit.Assert.assertEquals; nit: static imports usually come after normal ones. Line 38: @Test(timeout = 100000) Timeout for this can be pretty low, 15 seconds at max. Line 41: session.setFlushMode(SessionConfiguration.FlushMode.MANUAL_FLUSH); Can you also test with default mode? Line 65: assertEquals(0, actualRpcErrors); Can you test with errors? And batches with a mix of rows that are ok and rows with errors? Line 67: client.getTableClients().get(0).shutdown().join(DEFAULT_SLEEP); Not sure how this is necessary. -- 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: 2 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
