Dan Burkert has posted comments on this change. Change subject: [java-client] refactor AsyncKuduSession ......................................................................
Patch Set 4: (11 comments) 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: 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. 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 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 PleaseThrottleException). 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. 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<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. 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 form In the callsite where we had a variable (large) number of operations (Batch), we were copying into an array from an array list. 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 org.kududb.annotations.InterfaceAudience; > Unused? 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: 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
