This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch branch-1.15.x in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 04262dc8d20ac441890971d8487eda586fdbe43e Author: Bankim Bhavsar <[email protected]> AuthorDate: Mon May 24 17:34:14 2021 -0700 [master] KUDU-2181 Fix duplicate master address and remove unsafe flag tag While testing add master on a physical cluster, observed that supplying only hostnames without port resulted in duplicates being supplied to bring up new master which in turn leads to failure in creating distributed Raft config on startup. For e.g. kudu master add hp1,hp2 h2 Reason being hp2 is compared as hp2:7051 whereas in the vector of strings "master_addresses", it contains hp1,hp2. This changes adds a new function ParseAddresses() in HostPort class that's a variant of existing ParseStrings() function and takes a vector of strings instead. This new function is used in the duplicate detection logic. This change also removes the unsafe tag from --master_addr_add_new_master as the feature is ready but keeps it hidden as it's only meant to be used by the add master orchestration tool. This one is tricky to test locally and write a test because we need to specify ports for starting masters/tservers. Verified the fix on physical cluster. Along with CM integration we can add a systest later. Change-Id: Icf29730e3a6b225adb24ff161cac2ad777b46b81 Reviewed-on: http://gerrit.cloudera.org:8080/17500 Tested-by: Bankim Bhavsar <[email protected]> Reviewed-by: Alexey Serbin <[email protected]> (cherry picked from commit 261d71ef71fcfdf96c26c9d604d6fc0147c2ee6a) Reviewed-on: http://gerrit.cloudera.org:8080/17516 Tested-by: Kudu Jenkins --- src/kudu/master/sys_catalog.cc | 5 ++--- src/kudu/tools/tool_action_master.cc | 12 ++++++++++-- src/kudu/util/net/net_util.cc | 18 +++++++++++------- src/kudu/util/net/net_util.h | 4 ++++ 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc index 63aec50..f6946e4 100644 --- a/src/kudu/master/sys_catalog.cc +++ b/src/kudu/master/sys_catalog.cc @@ -88,11 +88,10 @@ DEFINE_double(sys_catalog_fail_during_write, 0.0, "Fraction of the time when system table writes will fail"); TAG_FLAG(sys_catalog_fail_during_write, hidden); -// Following flags related to dynamic multi-master are hidden till the feature is ready to -// be advertised. +// This flag is meant to be used by the tool orchestrating addition of a Kudu master and not +// for external documentation and hence it's hidden. DEFINE_string(master_address_add_new_master, "", "Address of master to add as a NON_VOTER on creating a distributed master config."); -TAG_FLAG(master_address_add_new_master, unsafe); TAG_FLAG(master_address_add_new_master, hidden); DECLARE_bool(master_support_change_config); diff --git a/src/kudu/tools/tool_action_master.cc b/src/kudu/tools/tool_action_master.cc index 9cf73d0..710903c 100644 --- a/src/kudu/tools/tool_action_master.cc +++ b/src/kudu/tools/tool_action_master.cc @@ -357,6 +357,12 @@ Status AddMaster(const RunnerContext& context) { // Parse input arguments. vector<string> master_addresses; RETURN_NOT_OK(ParseMasterAddresses(context, &master_addresses)); + // "master_addresses" after being successfully parsed above don't contain duplicates. + // So "master_hps" won't contain any duplicates as well. + vector<HostPort> master_hps; + RETURN_NOT_OK(HostPort::ParseAddresses(master_addresses, Master::kDefaultPort, &master_hps)); + DCHECK_EQ(master_addresses.size(), master_hps.size()); + const string& new_master_address = FindOrDie(context.required_args, kMasterAddressArg); HostPort hp; RETURN_NOT_OK(hp.ParseString(new_master_address, Master::kDefaultPort)); @@ -394,9 +400,11 @@ Status AddMaster(const RunnerContext& context) { // Bring up the new master that includes master addresses of the cluster and itself. // It's possible this is a retry in which case the new master is already part of // master_addresses. - if (std::find(master_addresses.begin(), master_addresses.end(), hp.ToString()) == - master_addresses.end()) { + // Using HostPort for comparing instead of strings as in case of latter, the default port + // may not be specified. + if (std::find(master_hps.begin(), master_hps.end(), hp) == master_hps.end()) { master_addresses.emplace_back(hp.ToString()); + master_hps.push_back(hp); } unique_ptr<Subprocess> new_master; diff --git a/src/kudu/util/net/net_util.cc b/src/kudu/util/net/net_util.cc index 87df153..edfa8e6 100644 --- a/src/kudu/util/net/net_util.cc +++ b/src/kudu/util/net/net_util.cc @@ -34,7 +34,7 @@ #include <utility> #include <vector> -#include <boost/functional/hash/hash.hpp> +#include <boost/container_hash/extensions.hpp> #include <gflags/gflags.h> #include <glog/logging.h> @@ -226,14 +226,18 @@ Status HostPort::ResolveAddresses(vector<Sockaddr>* addresses) const { Status HostPort::ParseStrings(const string& comma_sep_addrs, uint16_t default_port, vector<HostPort>* res) { - res->clear(); + return ParseAddresses(strings::Split(comma_sep_addrs, ",", strings::SkipEmpty()), + default_port, res); +} - vector<string> addr_strings = strings::Split(comma_sep_addrs, ",", strings::SkipEmpty()); - res->reserve(addr_strings.size()); - for (const string& addr_string : addr_strings) { +Status HostPort::ParseAddresses(const vector<string>& addrs, uint16_t default_port, + vector<HostPort>* res) { + res->clear(); + res->reserve(addrs.size()); + for (const string& addr : addrs) { HostPort host_port; - RETURN_NOT_OK(host_port.ParseString(addr_string, default_port)); - res->emplace_back(host_port); + RETURN_NOT_OK(host_port.ParseString(addr, default_port)); + res->emplace_back(std::move(host_port)); } return Status::OK(); } diff --git a/src/kudu/util/net/net_util.h b/src/kudu/util/net/net_util.h index 731d0ec..d29ca97 100644 --- a/src/kudu/util/net/net_util.h +++ b/src/kudu/util/net/net_util.h @@ -82,6 +82,10 @@ class HostPort { static Status ParseStrings( const std::string& comma_sep_addrs, uint16_t default_port, std::vector<HostPort>* res); + // Similar to above but uses a vector of strings 'addrs' as input parameter. + static Status ParseAddresses(const std::vector<std::string>& addrs, uint16_t default_port, + std::vector<HostPort>* res); + // Similar to above but allow the addresses to have scheme and path, // which are ignored. static Status ParseStringsWithScheme(
