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<Void>()).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<Void>()).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<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 
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<Operation> 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<Operation> 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 <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to