Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16894 )
Change subject: KUDU-2612 Java client transaction API ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/16894/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1181 PS2, Line 1181: This call is blocking > You mean we should follow the suite of majority of methods in this interfac I like the idea of keeping the deferred purity of the interface. I can imagine someone writing an async application that writes to kudu which asynchronously chains together opening, writing, and closing a transaction. It could also be useful to unify the handling of exceptions when making calls to this interface. 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? 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? 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 feature is ready for use? 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? 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? 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? 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 KuduTransaction next to the deserialize method? 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 transaction feature is missing/disabled server side? I think it would be good to test the auto close functionality too. -- 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 17:51:40 +0000 Gerrit-HasComments: Yes
