Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16974 )
Change subject: KUDU-2612: add a TxnSystemClient to the tservers ...................................................................... Patch Set 4: (7 comments) Overall looks good to me, just some nits. Thanks a lot Andrew! http://gerrit.cloudera.org:8080/#/c/16974/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16974/4//COMMIT_MSG@23 PS4, Line 23: connecting to nit: connecting to master? http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/rpc/sasl_common.cc@a280 PS4, Line 280: Why this is disabled? Sorry,I think you briefly mentioned offline, but I didn't quite get it. http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/transactions/txn_system_client.cc File src/kudu/transactions/txn_system_client.cc: http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/transactions/txn_system_client.cc@62 PS4, Line 62: disable_txn_system_client_init Should it be tagged as hidden as well? Do you expect this to be used outside test? http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/transactions/txn_system_client.cc@383 PS4, Line 383: if (PREDICT_FALSE(FLAGS_disable_txn_system_client_init)) { : KLOG_EVERY_N_SECS(WARNING, 60) << : Substitute("initialization of TxnSystemClient disabled, will retry in $0", : kRetryInterval.ToString()); : SleepFor(kRetryInterval); : continue; : } As we are retrying, do you expect FLAGS_disable_txn_system_client_init to change runtime? http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/transactions/txn_system_client.cc@390 PS4, Line 390: // HACK: if the master addresses are all totally unreachable, : // KuduClientBuilder::Build() will hang, attempting fruitlessly to : // retry, in the below call to Create(). So first, make sure we can at : // least reach the masters; if not, try again. : DnsResolver dns_resolver; : Status s; : for (const auto& hp : master_addrs) { : vector<Sockaddr> addrs; : s = dns_resolver.ResolveAddresses(hp, &addrs).AndThen([&] { : unique_ptr<MasterServiceProxy> proxy( : new MasterServiceProxy(messenger, addrs[0], hp.host())); : PingRequestPB req; : PingResponsePB resp; : RpcController rpc; : rpc.set_timeout(MonoDelta::FromSeconds(1)); : return proxy->Ping(req, &resp, &rpc); : }); : if (s.ok()) { : break; : } : } I guess the sanitize check is sufficient enough, even later when we actually call Create() all masters become unreachable? http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/transactions/txn_system_client.cc@435 PS4, Line 435: txn_client_ Do we want to check(dcheck) txn_client_ is not null here? http://gerrit.cloudera.org:8080/#/c/16974/4/src/kudu/transactions/txn_system_client.cc@443 PS4, Line 443: Wait nit: why not Shutdown() after Wait()? -- To view, visit http://gerrit.cloudera.org:8080/16974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33b5a2bb5c56ae4bb4b42069af5813e2780fc4bc Gerrit-Change-Number: 16974 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 27 Jan 2021 00:10:01 +0000 Gerrit-HasComments: Yes
