Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17868 )
Change subject: [consensus] KUDU-1620: re-resolve consensus peers on network error ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/consensus/consensus_peers.cc@585 PS3, Line 585: CreateConsensusServiceProxyForHost > I see this method is used in SetPermanentUuidForRemotePeer(), and the code SetPermanentUuidForRemotePeer() retries until success, up to a timeout. This patch actually fixes the behavior, in that it allows the proxy to re-resolve the addresses used, eventually succeeding if there was some SNAFU in the initial DNS resolution. Added a test for this case. http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/mini-cluster/external_mini_cluster.h@672 PS3, Line 672: SetRpcBindAddres > style nit: I see that there is rpc_bind_address() method (it's private, tho Done http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/mini-cluster/mini_cluster.h File src/kudu/mini-cluster/mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/mini-cluster/mini_cluster.h@145 PS3, Line 145: uint16_ > Port numbers can be up to 65535, so this should be uint16_t if we don't wan Done http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/rpc/proxy.h File src/kudu/rpc/proxy.h: http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/rpc/proxy.h@178 PS3, Line 178: std::shared_ptr<Messenger> messenger_; > nit: if this extra string isn't added intentionally, maybe remove it to kee Done http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/rpc/proxy.cc@229 PS3, Line 229: er that warrants a retry. : if (PREDICT_FALSE(!controller->status().ok())) { > nit: is it possible to use Substitute() to format the message? Done http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/17868/3/src/kudu/util/net/net_util.cc@225 PS3, Line 225: const auto > nit: is it crucial to have const reference here when GetCommandLineFlagInfo Done -- To view, visit http://gerrit.cloudera.org:8080/17868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd1b68c3c14d7d8f81168e16fe450d2ffcce840b Gerrit-Change-Number: 17868 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: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 06 Oct 2021 23:34:26 +0000 Gerrit-HasComments: Yes
