Todd Lipcon 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/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 2150: public Statistics getStatistics(){ > Oh I just thought of something, should we add a way to disable the collecti are you worried it might be too slow? Line 2150: s(){ nit: space between() and { 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 45: private long rowOperationsSize = 0; add a javadoc for this field 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 expect this to be used in that state? Line 146: statistics.incrementStatistic(tableName, tabletId, Statistic.OPTS_ERRORS, this.ops.size()); all of these incrementStatistic() calls are going to end up with redundant hashmap lookups against the same tableName/tabletId. Could we optimize this a bit by doing something like: TabletStatistics stats = statistics.getTabletStatistics(tableName, tabletId); stats.increment(Statistic.OPTS_ERRORS, this.ops.size()); ... so that we only do the hashmap lookup once? http://gerrit.cloudera.org:8080/#/c/2858/2/java/kudu-client/src/main/java/org/kududb/client/Operation.java File java/kudu-client/src/main/java/org/kududb/client/Operation.java: Line 180: String tabletId = this.getTablet().getTabletIdAsString(); same 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 146: private class TabletStatistics { shouldn't this be a private static class? Line 147: final private List<AtomicLong> statistics; how about AtomicLongArray here to avoid the extra object overheads, and pack the stats onto a smaller number of cache lines since they'll usually get updated together? -- 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
