Adar Dembo has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession

Patch Set 3:


Achievement unlocked: asynchronous programming wizard.
File java/kudu-client/src/main/java/org/kududb/client/

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.
File java/kudu-client/src/main/java/org/kududb/client/

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 

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 
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 
             :       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<List<BatchResponse>> 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 

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?
File java/kudu-client/src/main/java/org/kududb/client/

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

Line 46:   final List<Operation> operations = new ArrayList<>(1250);
What's magical about 1250?
File java/kudu-client/src/main/java/org/kududb/client/

Line 203:   static Tserver.WriteRequestPB.Builder 
createAndFillWriteRequestPB(List<Operation> operations) {
What motivated the changes here from variadic args to lists? Isn't the former 
(marginally) more performant?
File java/kudu-client/src/main/java/org/kududb/util/

Line 22: import java.util.List;

To view, visit
To unsubscribe, visit

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

Reply via email to