[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

eric-maynard has posted comments on this change.

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


Patch Set 5:

The build error seems unrelated (?):

09:29:53   Error: chose Boost outside
09:29:53   
/home/jenkins-slave/workspace/kudu-master/0/thirdparty/installed/common:
09:29:53   
/home/jenkins-slave/workspace/kudu-master/0/thirdparty/installed/uninstrumented/include
09:29:53 
09:29:53 
09:29:53 -- Configuring incomplete, errors occurred!

-- 
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: eric-maynard 
Gerrit-HasComments: No

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

eric-maynard has posted comments on this change.

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


Patch Set 5:

Added trimming when error queue capacity is decreased

-- 
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: eric-maynard 
Gerrit-HasComments: No

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 2:

(1 comment)

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.
> My thinking is that you'd actually want to specify the number of errors you
I can see the argument for both size-based and count-based capacity.

We used sized-based capacity in the C++ client at the request of Impala, who 
wanted to upper bound the memory consumption of a particular query against 
Kudu. Writ large, I think size-based capacity makes sense when the user of the 
client is an application.

On the other hand, as Eric points out, count-based capacity can be useful when 
the user of the client is a user who expects to interact directly with errors 
rather than adapting them into something else.

Another option is to implement both approaches and consider either bound when 
collecting a new error. What do you guys think?


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: eric-maynard 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5291

to look at the new patch set (#4).

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

KUDU-1422 [java client] modifiable error collector capacity

Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java
2 files changed, 25 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5291/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: eric-maynard 

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

eric-maynard has posted comments on this change.

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


Patch Set 2:

(1 comment)

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 ty
My thinking is that you'd actually want to specify the number of errors you're 
capturing, not necessarily the size of the buffer that stores the RowError 
objects (since the string message of the RowError can be any arbitrary length, 
and therefore a given RowError can be any arbitrary size). I know that if I 
wanted to capture the first 10 errors, I would have no idea what to set the 
memory usage to. 

I can see how you also might want to limit the memory usage of the buffer 
though. I'm not sure which approach I think is better.


-- 
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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: eric-maynard 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5291

to look at the new patch set (#3).

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

KUDU-1422 [java client] modifiable error collector capacity

Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java
2 files changed, 23 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5291/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

eric-maynard has posted comments on this change.

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


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5291/1/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 207: this.mutationBufferSpace = size;
> still think it should be a separate setter.
Done


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 wonde
Good idea about using {{setMaxCapacity}}. I'm not sure why you would want a 
size of 0, but it wouldn't break anything like using a negative number would so 
we can make that change if you think it's best.


Line 79: this.maxCapacity = maxCapacity;
> if you're reducing capacity, would make sense to trim the current queue dow
As it is now, you'd be prevented from adding errors after lowering the 
capacity, but you wouldn't throw out any existing errors that might be in the 
queue. I'm not sure if you'd want to dispose of errors you'd already captured, 
but if you'd prefer it that way let me know.


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: eric-maynard 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5291

to look at the new patch set (#2).

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

KUDU-1422 [java client] modifiable error collector capacity

Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java
2 files changed, 23 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5291/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

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


Patch Set 1:

(5 comments)

should also add a test case for this patch

http://gerrit.cloudera.org:8080/#/c/5291/1/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 207:   this.errorCollector.setMaxCapacity(size);
still think it should be a separate setter.


http://gerrit.cloudera.org:8080/#/c/5291/1/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 69:   
nit: remove trailing whitespace


PS1, Line 73: RuntimeException
should throw illegal argument exception


Line 76: if (maxCapacity > 0) {
typically we check for error cases first and "early exit", like:

if (maxCapacity <= 0) {
  throw ...
}
this.maxCapacity = maxCapacity

this style makes it so that the "normal path" is the one that's least indented.


PS1, Line 79:   else {
:   throw new RuntimeException("Need to be able to store at 
least one row error");
:   }
nit: style issues here (no tabs, fix indentation)


-- 
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1422 [java client] modifiable error collector capacity

[eric-maynard (Code Review)]

eric-maynard has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5291

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

KUDU-1422 [java client] modifiable error collector capacity

Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java
2 files changed, 16 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/5291/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iae382050eb5eaf9c169dd3ef7ed4dbf5b5f1c334
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard