Alexey Serbin 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 3: (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 there seems to assume the method behaves synchronously. With this change, do we expect any unexpected consequences when CreateConsensusServiceProxyForHost() is called by SetPermanentUuidForRemotePeer() when the initial DNS resolution fails? 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: set_rpc_hostport style nit: I see that there is rpc_bind_address() method (it's private, though). With that, maybe naming this new method SetRpcBindAddress() would be more appropriate? 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: int16_t Port numbers can be up to 65535, so this should be uint16_t if we don't want to introduce any artificial restrictions. 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: nit: if this extra string isn't added intentionally, maybe remove it to keep the file as is? 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: "Call had error, refreshing address and retrying: " : << controller->status().ToString(); nit: is it possible to use Substitute() to format the message? 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 GetCommandLineFlagInfoOrDie() returns a copy anyways? Maybe, it would be less surprising to read the NOTE above and the code below if this was just a const value? -- 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: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 04 Oct 2021 22:44:58 +0000 Gerrit-HasComments: Yes
