[kudu-CR] [java-client] refactor AsyncKuduSession

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 7:

Should we change the default buffer size up by an order of magnitude so that 
users don't experience this as a regression? We should also release note the 
changed client behavior.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: [java-client] refactor AsyncKuduSession
..


[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Reviewed-on: http://gerrit.cloudera.org:8080/3477
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
9 files changed, 592 insertions(+), 638 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] refactor AsyncKuduSession

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Kudu Jenkins,

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

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

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

Change subject: [java-client] refactor AsyncKuduSession
..

[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
9 files changed, 592 insertions(+), 638 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/3477/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3477
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 6:

Build Started http://104.196.14.100/job/kudu-gerrit/2121/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3477/5/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 405:   // no-op.
Can you make sure we have a test for this?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 563: doFlush(fullBuffer);
> I think doFlush() may throw (unchecked excpetions) if the buffer passed to 
I've changed doFlush to be a no-op when the buffer is empty just to make sure 
that a PTE callback can not be executed.


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Batch.java
File java/kudu-client/src/main/java/org/kududb/client/Batch.java:

Line 34: import org.kududb.tserver.Tserver;
> Odd that this comes after Tserver.TabletServerErrorPB.
Done


Line 46:   final List operations = new ArrayList<>(1250);
> So why bother predicting it at all?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2118/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Kudu Jenkins,

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

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

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

Change subject: [java-client] refactor AsyncKuduSession
..

[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
8 files changed, 590 insertions(+), 638 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/3477/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3477
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] refactor AsyncKuduSession

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 563: doFlush(fullBuffer);
> Not sure, but I don't think doFlush can throw.  In general, methods that re
I think doFlush() may throw (unchecked excpetions) if the buffer passed to it 
was empty, in which case it'd callback() on the global flushNotification 
deferred, which would potentially run user code that would throw.

It doesn't seem like that could happen from here (where fullBuffer is, well, 
full) but I wonder what that means for other doFlush() callers.


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Batch.java
File java/kudu-client/src/main/java/org/kududb/client/Batch.java:

Line 46:   final List operations = new ArrayList<>(125);
> nothing, and in fact it should probably be more like 125 (default buffer si
So why bother predicting it at all?

Anyway, if you do retain the preallocation, please add a comment explaining why 
125 was chosen.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 4: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2096/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 4: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2094/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 409: Deferred batchResponses = new Deferred<>();
> Nit: not really a fan of sometimes using Deferred.fromResult() and other ti
Here is not a case with a result though, it's just creating a Deferred.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 3:

(16 comments)

Achievement unlocked: asynchronous programming wizard.

http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 1410:Arrays.copyOf(partitionKey, 
partitionKey.length + 1), deadline);
So this is how we obtain the very next value following partitionKey, right? 
Surprised it's so simple given how complicated 
EncodedKey::IncrementEncodedKey() is.


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 123:* application is {@link #apply}ing operations to one buffer (the 
{@code activeBuffer}), the
Nit: {@link #apply} or {@code apply}? I see both here, not sure which is more 
correct.


Line 131:   private Buffer activeBuffer;
Like the C++ client's batcher abstraction? Nice.


Line 283: return flush();
Why flush() if the session is already closed?


PS3, Line 350:   buffer.callbackFlushNotification();
 :   inactiveBuffers.add(buffer);
 :   flushNotification.getAndSet(new 
Deferred()).callback(null);
And here there's the same sequence of events as in doFlush(), but in a 
different order. Why?


PS3, Line 402:   inactiveBuffers.add(buffer);
 :   // Send buffer available notification.
 :   flushNotification.getAndSet(new 
Deferred()).callback(null);
 :   buffer.callbackFlushNotification();
AFAICT, there's no lock held for this work. The order strikes me as strange: 
we're adding buffer to inactiveBuffers (thus making it globally visible) before 
invoking callbacks, especially the buffer's own callback. Is this safe?


Line 409: Deferred batchResponses = new Deferred<>();
Nit: not really a fan of sometimes using Deferred.fromResult() and other times 
invoking the Deferred constructor directly. Can we be more consistent?


PS3, Line 425: private static final ConvertBatchToListOfResponsesCB INSTANCE =
 : new ConvertBatchToListOfResponsesCB();
Is this for correctness or performance? Seems like the implementation would be 
simpler if we didn't maintain a singleton.


Line 563: doFlush(fullBuffer);
What happens if doFlush() throws but we're executing it because we throw a 
PleaseThrottle or NonRecoverable exception in flush()?


Line 704:   private final class Buffer {
Shouldn't Buffer be declared static? Or did you intend for it to be an inner 
class? I don't see any accesses to the outer class; maybe I missed them though.

Nit: not really clear on why you'd use final here. I don't think there's any 
real danger of someone extending Buffer, so final just adds noise, no?


Line 748:   flushNotification = new Deferred<>();
Nit: use fromResult(null) here to be consistent with the inline field 
constructor.

Better yet, perhaps add a no-arg constructor that initializes via reset()? Then 
it's even more consistent.


Line 756: .add("flusherTask", flusherTask)
This will just be an object address, right? Useful?


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Batch.java
File java/kudu-client/src/main/java/org/kududb/client/Batch.java:

Line 34: import org.kududb.tserver.Tserver;
Odd that this comes after Tserver.TabletServerErrorPB.


Line 46:   final List operations = new ArrayList<>(1250);
What's magical about 1250?


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Operation.java
File java/kudu-client/src/main/java/org/kududb/client/Operation.java:

Line 203:   static Tserver.WriteRequestPB.Builder 
createAndFillWriteRequestPB(List operations) {
What motivated the changes here from variadic args to lists? Isn't the former 
(marginally) more performant?


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
File java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java:

Line 22: import java.util.List;
Unused?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java-client] refactor AsyncKuduSession
..

[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
8 files changed, 593 insertions(+), 639 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 2:

(1 comment)

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

PS2, Line 396: Must not be concurrently accessed
> I'm still thrown off by this comment. Maybe:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2038/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 2:

(1 comment)

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

PS2, Line 396: Must not be concurrently accessed
I'm still thrown off by this comment. Maybe:
* @param buffer the buffer to flush, must not be modified once passed to this 
method


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

PS1, Line 1400: locateTablet
> How does this related to the other locateTablet() method? It seems to be do
Done


http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

PS1, Line 396: Must not be concurrently accessed
> It already is it seems though, the client can call it with a flusher task f
The flusher task and the client task both lock the monitor and set the active 
buffer to null before calling this, so I don't think it's possible.


Line 705: private List operations = new ArrayList<>();
> final
Done


PS1, Line 725: (
> not closing the parenthesis in the comment
Done


http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/client/RowError.java
File java/kudu-client/src/main/java/org/kududb/client/RowError.java:

Line 87: ", tablet=" + (operation.getTablet() == null ? null :
> Just put getTablet there directly, RemoteTablet has toString.
Done


http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
File java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java:

Line 54:* Workaround for {@link Deferred#addBoth}'s failure to use generics 
correctly.
> Can you be more clear regarding what this does and why someone should use i
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java-client] refactor AsyncKuduSession
..

[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
8 files changed, 593 insertions(+), 639 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/1984/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

PS1, Line 1400: locateTablet
How does this related to the other locateTablet() method? It seems to be doing 
something different, so at least it shouldn't be called the same?


http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

PS1, Line 396: Must not be concurrently accessed
It already is it seems though, the client can call it with a flusher task fires.


Line 705: private List operations = new ArrayList<>();
final


PS1, Line 725: it's
its


PS1, Line 725: (
not closing the parenthesis in the comment


http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/client/RowError.java
File java/kudu-client/src/main/java/org/kududb/client/RowError.java:

Line 87: ", tablet=" + (operation.getTablet() == null ? null :
Just put getTablet there directly, RemoteTablet has toString.


http://gerrit.cloudera.org:8080/#/c/3477/1/java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
File java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java:

Line 54:* Workaround for {@link Deferred#addBoth}'s failure to use generics 
correctly.
Can you be more clear regarding what this does and why someone should use it?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: [java-client] refactor AsyncKuduSession
..

[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
8 files changed, 587 insertions(+), 638 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans