Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17343 )
Change subject: [client] retry master RPCs on network errors ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/client/txn_manager_proxy_rpc.cc File src/kudu/client/txn_manager_proxy_rpc.cc: http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/client/txn_manager_proxy_rpc.cc@110 PS1, Line 110: auto proxy = client_->data_->txn_manager_proxy().lock(); : if (!proxy) { : SendRpcCb(Status::IllegalState("proxy has been destructed")); : return; : } How much is it differnt from returning std::shared_ptr by txn_manager_proxy() and keeping local reference here at stack? I guess my question is what's the benefit of returning std::weak_ptr by txn_manager_proxy()? http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/client/txn_manager_proxy_rpc.cc@115 PS1, Line 115: func_(proxy.get() The same pattern is in AsyncLeaderMasterRpc<ReqClass, RespClass>::SendRpc(), file master_proxy_rpc.cc. Does it make sense to update it as well? http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/integration-tests/txn_status_table-itest.cc File src/kudu/integration-tests/txn_status_table-itest.cc: http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/integration-tests/txn_status_table-itest.cc@477 PS1, Line 477: ASSERT_OK Just curious: does this work as expected when Restart() returns non-OK status? I recall there were issues with using ASSERT_OK() in threads, so CHECK_OK() was used instead. http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/integration-tests/txn_status_table-itest.cc@486 PS1, Line 486: MonoDelta::FromSeconds(3))); Just curios: does this turn to be enough to be stable in case of TSAN build and a heavy loaded node (say, if running the test with --stress_cpu_threads=16)? http://gerrit.cloudera.org:8080/#/c/17343/1/src/kudu/integration-tests/txn_status_table-itest.cc@487 PS1, Line 487: 1 nit: is this some sort of invariant we want to enforce in this scenario or it's rather important to see any valid transaction ID here? It would be nice to have a comment about this. -- To view, visit http://gerrit.cloudera.org:8080/17343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae2febb9e890acf6f7efd5cce3cb7e4f7b5f683d Gerrit-Change-Number: 17343 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 27 Apr 2021 17:57:34 +0000 Gerrit-HasComments: Yes
