[kudu-CR] KUDU-1415. Add statistics in java client.

["Todd Lipcon (Code Review)" <ge...@cloudera.org>]

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 <zh...@xiaomi.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: zhen.zhang <zh...@xiaomi.com>
Gerrit-HasComments: Yes