Dan Burkert has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession

Patch Set 4:


File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 1410:       locatedTablets = locateTable(table, partitionKey,
> So this is how we obtain the very next value following partitionKey, right?
yes.  IncrementEncodedKey is more complicated because it has to produce a valid 
key that could be decoded.  This key doesn't need to be decoded, so it doesn't 
have to be as complicated.  For instance, if the key was just a single int32 
column this would result in 5 bytes, but it's only used for comparing so it 
doesn't matter.

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 mo
I try and use @link for the first reference and @code thereafter.

Line 283:     return flush();
> Why flush() if the session is already closed?
flush() is already a no-op if the active buffer is null.

PS3, Line 425: private static final ConvertBatchToListOfResponsesCB INSTANCE =
             :         new ConvertBatchToListOfResponsesCB();
> Is this for correctness or performance? Seems like the implementation would
It's just to save allocation overhead since this is a static class with no 
fields.  Removing the caching wouldn't change the logic in #call.

Line 563:         doFlush(fullBuffer);
> What happens if doFlush() throws but we're executing it because we throw a 
Not sure, but I don't think doFlush can throw.  In general, methods that return 
Deferred shouldn't throw (the notable exception to this is 

Line 704:   private final class Buffer {
> Shouldn't Buffer be declared static? Or did you intend for it to be an inne
Flusher is non-static, so transitively Buffer must be non-static.  I use final 
because it's a signal that you don't have to worry about OO shenanigans.  
Otherwise you have to search the file to make sure.

Line 748:       flushNotification = new Deferred<>();
> Nit: use fromResult(null) here to be consistent with the inline field const
fromResult(null) creates a completed deferred, whereas new Deferred() creates 
an incomplete deferred.  When the buffer is created it's not flushing, so the 
flush notification should be complete. When it's reset it's being made into the 
active buffer, so the deferred is changed to an incomplete deferred.

Line 756:                         .add("flusherTask", flusherTask)
> This will just be an object address, right? Useful?
Yes, just to distinguish it from null.

File java/kudu-client/src/main/java/org/kududb/client/Batch.java:

Line 46:   final List<Operation> operations = new ArrayList<>(125);
> What's magical about 1250?
nothing, and in fact it should probably be more like 125 (default buffer size / 
8).  8 is similarly arbitrary, but seems like a reasonable number of tablets 
for a table.  There's really no way to predict how big this should be.

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 form
In the callsite where we had a variable (large) number of operations (Batch), 
we were copying into an array from an array list.

File java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java:

Line 22: import org.kududb.annotations.InterfaceAudience;
> 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: 4
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