Todd Lipcon has posted comments on this change.

Change subject: KUDU-1422 [java client] modifiable error collector capacity
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5291/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:

Line 213:    * Sets the maximum capacity of this session's ErrorCollector.
I'm a little nervous about this public API, come to think of it, because 
typically people want to bound memory usage, not bound the count of items (when 
they don't know how large they could be). That change is in-flight on the C++ 
side to go to memory-based error limiting, and I'm worried about introducing 
this API here if we know it's not the one we want to maintain long-term (APIs 
are forever).

Would you be interested in making the capacity size-based instead of 
count-based?


http://gerrit.cloudera.org:8080/#/c/5291/2/java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java:

Line 43:     Preconditions.checkArgument(maxCapacity > 0, "Error collector 
needs to be able to store at least one row error");
why not refactor this to use setMaxCapacity? Also, looking at this, I wonder 
why we don't support setting it to 0


Line 79:     this.maxCapacity = maxCapacity;
if you're reducing capacity, would make sense to trim the current queue down to 
the specified capacity, no?


-- 
To view, visit http://gerrit.cloudera.org:8080/5291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <emayn...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to