Todd Lipcon has posted comments on this change. Change subject: KUDU-1415. Add statistics in java client. ......................................................................
Patch Set 5: (18 comments) mostly nit-picks here http://gerrit.cloudera.org:8080/#/c/2858/5/java/kudu-client/src/main/java/org/kududb/client/Batch.java File java/kudu-client/src/main/java/org/kududb/client/Batch.java: Line 48: * operations in this batch. Also this size is used to throttle io on the server side. I don't think we should mention that this has anything to do with server-side throttling, since that's sort of an implementation detail, and not like the user can set this in order to change throttling behavior Line 153: String tabletId = this.getTablet().getTabletIdAsString(); this is going to be slow since each call here is creating a new string. Can we keep it as a Slice internally and only convert to String when we expose it back to the user, since we assume the user would be reading the statistics infrequently, but updating them frequently (with every op) Line 157: tabletStatistics.incrementStatistic(Statistic.OPTS_ERRORS, this.ops.size()); OPS_ERRORS not OPTS Line 166: tabletStatistics.incrementStatistic(Statistic.WRITE_OPTS, 1); WRITE_OPS not OPTS http://gerrit.cloudera.org:8080/#/c/2858/5/java/kudu-client/src/main/java/org/kududb/client/Operation.java File java/kudu-client/src/main/java/org/kududb/client/Operation.java: Line 55: * operation. Also this size is used to throttle io on the server side. same comment as elsewhere http://gerrit.cloudera.org:8080/#/c/2858/5/java/kudu-client/src/main/java/org/kududb/client/Statistics.java File java/kudu-client/src/main/java/org/kududb/client/Statistics.java: Line 30: , it new sentence: "AsyncKuduClient. It stores" Line 36: , us "thread-safe. The user can" Line 62: OPTS( should abbreviate as OPS not OPTS Line 85: public int getIndex() { does this need to be public? Line 103: this.stsMap.get(tabletId) can we store this as a variable above to avoid the double lookup? Line 111: the number of this type's the value of the statistic Line 126: specify t don't need to say 'specify' Line 127: number of this type's 'value of the statistic' Line 139: also s/also// Line 140: id ids Line 148: also s/also// Line 149: name names Line 156: return Collections.unmodifiableSet(tables); since you've constructed a new set here, it's safe to return it directly instead of making it unmodifiable -- 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: 5 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
