zhen.zhang has posted comments on this change. Change subject: KUDU-1415. Add statistics in java client. ......................................................................
Patch Set 2: (8 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 67: * @return bytes size or 0 if serialize is not called yet > should it throw IllegalStateException if serialize is not called yet? do we No, we don't expect this to be used before serialize is called. Now this method is only used when we got rpc's response, where we can assure serialize is already called. Line 146: statistics.incrementStatistic(tableName, tabletId, Statistic.OPTS_ERRORS, this.ops.size()); > all of these incrementStatistic() calls are going to end up with redundant OK, then this needs to expose TabletStatistics here. 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. OK, I'll write more details. Line 132: > We should also have methods that do rollups, else it's not going to be fun yes, I'll add methods to get client level and table level statistics. Line 146: private class TabletStatistics { > shouldn't this be a private static class? Maybe static but not private. As we want to use this class in Operation and Batch to avoid redundant hashmap lookups. Line 147: final private List<AtomicLong> statistics; > how about AtomicLongArray here to avoid the extra object overheads, and pac OK, AtomicLongArray is really a better choice. 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 41: session.setFlushMode(SessionConfiguration.FlushMode.MANUAL_FLUSH); > Can you also test with default mode? sure. Line 65: assertEquals(0, actualRpcErrors); > Can you test with errors? And batches with a mix of rows that are ok and ro sure. -- 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
