Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16929 )
Change subject: KUDU-2612 Java client transaction implementation ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/16929/4/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/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@504 PS4, Line 504: // We should have a UUID to construct ServerInfo for the master, but we have a chicken Instead of duplicating the newMasterRpcProxy can we just call newMasterRpcProxy here? http://gerrit.cloudera.org:8080/#/c/16929/4/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/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@292 PS4, Line 292: final Transactions.TxnStatePB txnState = resp.txnState(); Do we expose the transaction state publicly anywhere? It could be useful to get the state directly. Of course that means mapping the PB enum to a client side enum that is public (likely in the response object). http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@325 PS4, Line 325: synchronized (isInFlightSync) { Would it make sense to update this first to prevent other call to things like `newKuduSession` immediately? Same for the other places. Should we expect KuduTransaction to be threadsafe? http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@466 PS4, Line 466: // TODO(adar): By scheduling even the first RPC via timer, the sequence of Is this comment copied somewhere? Is it completely relevant here? http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@517 PS4, Line 517: // TODO(aserbin): use proper names Is this for a follow up? -- To view, visit http://gerrit.cloudera.org:8080/16929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0236875e7264877c3f7a13da4a5a3da6423786b Gerrit-Change-Number: 16929 Gerrit-PatchSet: 4 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-Comment-Date: Thu, 21 Jan 2021 14:40:09 +0000 Gerrit-HasComments: Yes
