Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16894 )
Change subject: KUDU-2612 Java client transaction API ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java: http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@43 PS2, Line 43: @c > typo? Done http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@47 PS2, Line 47: soon > when is soon explicitly? Done http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@50 PS2, Line 50: @InterfaceStability.Evolving > Should we mark this as InterfaceStability.Unstable until the transaction fe It's a good point, thanks. Done. http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@70 PS2, Line 70: this.client = client; > when does txnId and keepaliveMillis get set when using this constructor? This is done after receiving response for the BeginTransaction() RPC. I was planning to have the actual functionality in this patch, but after recent sync-up with Andrew and Hao there was a decision to keep this patch as a hollow API-only shell. The actual functionality comes in a follow-up patch. http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@248 PS2, Line 248: rolling back a transaction > Is close only called when rolling back? This is an override for AutoCloseable::close(). It's possible to call 'close()' explicitly, and it's totally fine to call 'KuduTransaction::rollback()' at any time explicitly when needed: e.g., when an exception is thrown by a write operation created from in transactional KuduSession. http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransactionSerializer.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransactionSerializer.java: http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransactionSerializer.java@108 PS2, Line 108: public void setEnableKeepalive(boolean enableKeepalive) { > Why isn't this toggled on the transaction itself? A couple of reasons: * in case of newly created transaction, the keepalive heartbeating should be automatically enabled always, so not much sense in such a control there * in case of the 'star' topology, when deserializing a transaction from a token there isn't much sense of sending keepalive messages for every txn created out of token once the transaction commit/rollback is controlled by the top-level txn (e.g., the transaction started by Spark driver), otherwise it will lead to duplicated keepalive messages Basically, the only case when it's necessary to enable keepalive for a transaction created by deserializing a txn token is the 'ring' topology. http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransactionSerializer.java@134 PS2, Line 134: public byte[] serialize() throws IOException { > What's the reason for a separate class for this versus putting in KuduTrans Because of the consistency reasons. (a) It's necessary to have the control over the keepalive property for a transaction deserialized from a token, where the creator of the token (not the caller of the deserialize() method) controls the keepalive behavior (b) adding serialize() into the KuduTransaction() along with keepalive control makes the deserialize() method inconsistent since it makes the caller think that deserialize() is also affected by the setEnableKeepalive() settings. http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java: http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java@31 PS2, Line 31: public class TestKuduTransaction { > Does it make sense to add a test for the expected behavior when the transac Yeah, many more tests are being added. I'll add the tests you mentioned in the follow-up patch with the actual functionality. Since PS3 this becomes a hollow API shell-only patch -- I'm moving all the tests out into the follow-up patch. -- To view, visit http://gerrit.cloudera.org:8080/16894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbb18e1ac0454a8ef9e3486430dfaa336e381e07 Gerrit-Change-Number: 16894 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 06 Jan 2021 20:33:21 +0000 Gerrit-HasComments: Yes
