Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16929 )
Change subject: KUDU-2612 Java client transaction implementation ...................................................................... Patch Set 4: (7 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 newMasterRpcP Yup -- it seems this newTxnManagerRpcProxy isn't needed since TxnManager calls reuse master proxies, i.e. the essence of RpcProxy is just a connection to particular master: the proxy doesn't differentiate between different RPC services behind the same TCP end-point. 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@325 PS4, Line 325: synchronized (isInFlightSync) { > Would it make sense to update this first to prevent other call to things li I guess it might make sense to add more guardrails w.r.t. stage changes, but it would complicate this code without much benefits due to all error handling, IMO. Even if they call newKuduSession on an already rolled back or committed transaction, it's not a problem -- there will be an error with any operation on such a session down the road. And even if there was such a guardrail in place, it would not help in case of using de-serialized handle when the underlying transaction is committed/rolledback anyways. Yes, I think we want KuduTransaction to be threadsafe. http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@468 PS4, Line 468: where the table is already fully altered. > nit: where the transaction is fully committed? Done 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? Whoops -- this is addressed already, thanks. http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java File java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java: http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@296 PS4, Line 296: // TODO(aserbin): implement this > nit: remove? Or is there more to be done? Ah, indeed -- this is now implemented http://gerrit.cloudera.org:8080/#/c/16929/4/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java@440 PS4, Line 440: // TODO(aserbin): try sending request to other TxnManager instance, if possible > Is the 'connection' here compatible with using client.handleNotLeader()? I Right -- 'connection' has information on master leadership. However, "demoting" current leader for fake master tablet without knowing whether the leader has actually changed will affect other calls to master. Probably, that means using just another fake master tablet but just for TxnManager calls. I guess I want to say that this should be addressed in a proper way, I'm thinking to post a follow-up patch for that, if you don't mind having that addressed separately from this patch. http://gerrit.cloudera.org:8080/#/c/16929/4/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/16929/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java@401 PS4, Line 401: ou > nit: typo Done -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Comment-Date: Thu, 21 Jan 2021 23:29:50 +0000 Gerrit-HasComments: Yes
