You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2022/06/07 16:28:56 UTC
[trafficserver] branch 9.2.x updated: Change parent_select Init func to constructor (#8853)
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 9.2.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/9.2.x by this push:
new c9661f78b Change parent_select Init func to constructor (#8853)
c9661f78b is described below
commit c9661f78b58d210e5dfa2c64328854a6a0037b78
Author: Robert O Butts <ro...@users.noreply.github.com>
AuthorDate: Mon May 23 17:16:51 2022 -0600
Change parent_select Init func to constructor (#8853)
Tech-debt: makes it impossible to call Init multiple times
and produce a malformed object.
(cherry picked from commit aeb326304b1232bdc84afc588c4a36932cb58048)
---
.../experimental/parent_select/consistenthash.cc | 43 +++++-----------------
.../experimental/parent_select/consistenthash.h | 3 +-
.../parent_select/consistenthash_config.cc | 12 +++---
plugins/experimental/parent_select/strategy.cc | 38 ++-----------------
plugins/experimental/parent_select/strategy.h | 5 +--
proxy/http/remap/NextHopConsistentHash.cc | 14 ++-----
proxy/http/remap/NextHopConsistentHash.h | 4 +-
proxy/http/remap/NextHopRoundRobin.h | 8 ++--
proxy/http/remap/NextHopSelectionStrategy.cc | 36 ++++++------------
proxy/http/remap/NextHopSelectionStrategy.h | 5 +--
proxy/http/remap/NextHopStrategyFactory.cc | 38 +++++++++----------
11 files changed, 63 insertions(+), 143 deletions(-)
diff --git a/plugins/experimental/parent_select/consistenthash.cc b/plugins/experimental/parent_select/consistenthash.cc
index f2f8587ea..d26eed5a6 100644
--- a/plugins/experimental/parent_select/consistenthash.cc
+++ b/plugins/experimental/parent_select/consistenthash.cc
@@ -106,8 +106,6 @@ PLNextHopConsistentHash::chashLookup(const std::shared_ptr<ATSConsistentHash> &r
}
}
-PLNextHopConsistentHash::PLNextHopConsistentHash(const std::string_view name) : PLNextHopSelectionStrategy(name) {}
-
PLNextHopConsistentHash::~PLNextHopConsistentHash()
{
PL_NH_Debug(PL_NH_DEBUG_TAG, "destructor called for strategy named: %s", strategy_name.c_str());
@@ -115,10 +113,10 @@ PLNextHopConsistentHash::~PLNextHopConsistentHash()
#define PLUGIN_NAME "pparent_select"
-bool
-PLNextHopConsistentHash::Init(const YAML::Node &n)
+PLNextHopConsistentHash::PLNextHopConsistentHash(const std::string_view name, const YAML::Node &n)
+ : PLNextHopSelectionStrategy(name, n)
{
- TSDebug("pparent_select", "PLNextHopConsistentHash Init calling.");
+ TSDebug("pparent_select", "PLNextHopConsistentHash constructor calling.");
ATSHash64Sip24 hash;
@@ -144,24 +142,10 @@ PLNextHopConsistentHash::Init(const YAML::Node &n)
}
}
} catch (std::exception &ex) {
- TSDebug(PLUGIN_NAME, "Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(),
- ex.what());
-
- PL_NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(),
- ex.what());
- return false;
+ throw std::invalid_argument("Error parsing the strategy named '" + strategy_name + "' due to '" + ex.what() +
+ "', this strategy will be ignored.");
}
- TSDebug(PLUGIN_NAME, "PLNextHopConsistentHash::Init strat Init calling.");
-
- bool result = PLNextHopSelectionStrategy::Init(n);
- if (!result) {
- TSDebug(PLUGIN_NAME, "PLNextHopConsistentHash::Init strat Init false.");
- return false;
- }
-
- TSDebug(PLUGIN_NAME, "PLNextHopConsistentHash::Init strat Init called.");
-
// load up the hash rings.
for (uint32_t i = 0; i < groups; i++) {
std::shared_ptr<ATSConsistentHash> hash_ring = std::make_shared<ATSConsistentHash>();
@@ -187,22 +171,15 @@ PLNextHopConsistentHash::Init(const YAML::Node &n)
if (ring_mode == PL_NH_PEERING_RING) {
if (groups == 1) {
if (!go_direct) {
- PL_NH_Error("when ring mode is '%s', go_direct must be true when there is only one host group.", peering_rings.data());
- return false;
+ throw std::invalid_argument("ring mode '" + std::string(peering_rings) +
+ "' go_direct must be true when there is only one host group");
}
} else if (groups != 2) {
- PL_NH_Error("when ring mode is '%s', requires two host groups (peering group and an upstream group),"
- " or just a single peering group with go_direct.",
- peering_rings.data());
- return false;
+ throw std::invalid_argument(
+ "ring mode '" + std::string(peering_rings) +
+ "' requires two host groups (peering group and an upstream group), or a single peering group with go_direct");
}
- // if (policy_type != PL_NH_CONSISTENT_HASH) {
- // PL_NH_Error("ring mode '%s', is only implemented for a 'consistent_hash' policy.", peering_rings.data());
- // return false;
- // }
}
-
- return true;
}
// returns a hash key calculated from the request and 'hash_key' configuration
diff --git a/plugins/experimental/parent_select/consistenthash.h b/plugins/experimental/parent_select/consistenthash.h
index 6232f07fc..c24a2fb4e 100644
--- a/plugins/experimental/parent_select/consistenthash.h
+++ b/plugins/experimental/parent_select/consistenthash.h
@@ -73,9 +73,8 @@ public:
PLNHHashKeyType hash_key = PL_NH_PATH_HASH_KEY;
PLNextHopConsistentHash() = delete;
- PLNextHopConsistentHash(const std::string_view name);
+ PLNextHopConsistentHash(const std::string_view name, const YAML::Node &n);
~PLNextHopConsistentHash();
- bool Init(const YAML::Node &n);
void next(TSHttpTxn txnp, void *strategyTxn, const char *exclude_hostname, size_t exclude_hostname_len, in_port_t exclude_port,
const char **out_hostname, size_t *out_hostname_len, in_port_t *out_port, bool *out_retry, bool *out_no_cache,
time_t now = 0) override;
diff --git a/plugins/experimental/parent_select/consistenthash_config.cc b/plugins/experimental/parent_select/consistenthash_config.cc
index d33489694..cf63d83ab 100644
--- a/plugins/experimental/parent_select/consistenthash_config.cc
+++ b/plugins/experimental/parent_select/consistenthash_config.cc
@@ -66,14 +66,14 @@ TSNextHopSelectionStrategy *
createStrategy(const std::string &name, const YAML::Node &node)
{
TSDebug(PLUGIN_NAME, "createStrategy %s calling.", name.c_str());
- PLNextHopConsistentHash *st = new PLNextHopConsistentHash(name);
- TSDebug(PLUGIN_NAME, "createStrategy %s newed %p.", name.c_str(), (void *)st);
- if (!st->Init(node)) {
- TSDebug(PLUGIN_NAME, "createStrategy %s init failed, returning nullptr.", name.c_str());
+ try {
+ PLNextHopConsistentHash *st = new PLNextHopConsistentHash(name, node);
+ TSDebug(PLUGIN_NAME, "createStrategy %s succeeded, returning object", name.c_str());
+ return st;
+ } catch (std::exception &ex) {
+ TSError("[%s] creating strategies '%s' threw '%s', returning nullptr", PLUGIN_NAME, name.c_str(), ex.what());
return nullptr;
}
- TSDebug(PLUGIN_NAME, "createStrategy %s init succeeded, returning obj.", name.c_str());
- return st;
}
// Caller takes ownership of the returned pointers in the map, and must call delete on them.
diff --git a/plugins/experimental/parent_select/strategy.cc b/plugins/experimental/parent_select/strategy.cc
index 270ff8cc8..385f04c0e 100644
--- a/plugins/experimental/parent_select/strategy.cc
+++ b/plugins/experimental/parent_select/strategy.cc
@@ -55,18 +55,9 @@ const std::string_view peering_rings = "peering_ring";
const std::string_view active_health_check = "active";
const std::string_view passive_health_check = "passive";
-PLNextHopSelectionStrategy::PLNextHopSelectionStrategy(const std::string_view &name)
+PLNextHopSelectionStrategy::PLNextHopSelectionStrategy(const std::string_view &name, const YAML::Node &n) : strategy_name(name)
{
- strategy_name = name;
-}
-
-//
-// parse out the data for this strategy.
-//
-bool
-PLNextHopSelectionStrategy::Init(const YAML::Node &n)
-{
- PL_NH_Debug(PL_NH_DEBUG_TAG, "calling Init()");
+ PL_NH_Debug(PL_NH_DEBUG_TAG, "PLNextHopSelectionStrategy constructor calling");
std::string self_host;
bool self_host_used = false;
@@ -249,30 +240,9 @@ PLNextHopSelectionStrategy::Init(const YAML::Node &n)
throw std::invalid_argument("self host (" + self_host + ") does not appear in the first (peer) group");
}
} catch (std::exception &ex) {
- PL_NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(),
- ex.what());
- return false;
+ throw std::invalid_argument("Error parsing strategy named '" + strategy_name + "' due to '" + ex.what() +
+ "', this strategy will be ignored.");
}
-
- if (ring_mode == PL_NH_PEERING_RING) {
- if (groups == 1) {
- if (!go_direct) {
- PL_NH_Error("when ring mode is '%s', go_direct must be true when there is only one host group.", peering_rings.data());
- return false;
- }
- } else if (groups != 2) {
- PL_NH_Error("when ring mode is '%s', requires two host groups (peering group and an upstream group),"
- " or just a single peering group with go_direct.",
- peering_rings.data());
- return false;
- }
- // if (policy_type != PL_NH_CONSISTENT_HASH) {
- // PL_NH_Error("ring mode '%s', is only implemented for a 'consistent_hash' policy.", peering_rings.data());
- // return false;
- // }
- }
-
- return true;
}
bool
diff --git a/plugins/experimental/parent_select/strategy.h b/plugins/experimental/parent_select/strategy.h
index 6a8a6b6b6..e3cd47aa2 100644
--- a/plugins/experimental/parent_select/strategy.h
+++ b/plugins/experimental/parent_select/strategy.h
@@ -240,10 +240,9 @@ public:
class PLNextHopSelectionStrategy : public TSNextHopSelectionStrategy
{
public:
- PLNextHopSelectionStrategy();
- PLNextHopSelectionStrategy(const std::string_view &name);
+ PLNextHopSelectionStrategy() = delete;
+ PLNextHopSelectionStrategy(const std::string_view &name, const YAML::Node &n);
virtual ~PLNextHopSelectionStrategy(){};
- bool Init(const YAML::Node &n);
virtual void next(TSHttpTxn txnp, void *strategyTxn, const char *exclude_hostname, size_t exclude_hostname_len,
in_port_t exclude_port, const char **out_hostname, size_t *out_hostname_len, in_port_t *out_port,
diff --git a/proxy/http/remap/NextHopConsistentHash.cc b/proxy/http/remap/NextHopConsistentHash.cc
index b3f68a517..c12773ce6 100644
--- a/proxy/http/remap/NextHopConsistentHash.cc
+++ b/proxy/http/remap/NextHopConsistentHash.cc
@@ -83,8 +83,8 @@ NextHopConsistentHash::~NextHopConsistentHash()
NH_Debug(NH_DEBUG_TAG, "destructor called for strategy named: %s", strategy_name.c_str());
}
-bool
-NextHopConsistentHash::Init(ts::Yaml::Map &n)
+NextHopConsistentHash::NextHopConsistentHash(const std::string_view name, const NHPolicyType &policy, ts::Yaml::Map &n)
+ : NextHopSelectionStrategy(name, policy, n)
{
ATSHash64Sip24 hash;
@@ -110,13 +110,8 @@ NextHopConsistentHash::Init(ts::Yaml::Map &n)
}
}
} catch (std::exception &ex) {
- NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
- return false;
- }
-
- bool result = NextHopSelectionStrategy::Init(n);
- if (!result) {
- return false;
+ throw std::invalid_argument("Error parsing the strategy named '" + strategy_name + "' due to '" + ex.what() +
+ "', this strategy will be ignored.");
}
// load up the hash rings.
@@ -140,7 +135,6 @@ NextHopConsistentHash::Init(ts::Yaml::Map &n)
hash.clear();
rings.push_back(std::move(hash_ring));
}
- return true;
}
// returns a hash key calculated from the request and 'hash_key' configuration
diff --git a/proxy/http/remap/NextHopConsistentHash.h b/proxy/http/remap/NextHopConsistentHash.h
index 7802fdc20..f439352e9 100644
--- a/proxy/http/remap/NextHopConsistentHash.h
+++ b/proxy/http/remap/NextHopConsistentHash.h
@@ -47,9 +47,9 @@ public:
NHHashKeyType hash_key = NH_PATH_HASH_KEY;
NextHopConsistentHash() = delete;
- NextHopConsistentHash(const std::string_view name, const NHPolicyType &policy) : NextHopSelectionStrategy(name, policy) {}
+ NextHopConsistentHash(const std::string_view name, const NHPolicyType &policy, ts::Yaml::Map &n);
~NextHopConsistentHash();
- bool Init(ts::Yaml::Map &n);
+
void findNextHop(TSHttpTxn txnp, void *ih = nullptr, time_t now = 0) override;
std::shared_ptr<HostRecord> chashLookup(const std::shared_ptr<ATSConsistentHash> &ring, uint32_t cur_ring, ParentResult &result,
HttpRequestData &request_info, bool *wrapped, uint64_t sm_id);
diff --git a/proxy/http/remap/NextHopRoundRobin.h b/proxy/http/remap/NextHopRoundRobin.h
index 024efee70..a5d08c7df 100644
--- a/proxy/http/remap/NextHopRoundRobin.h
+++ b/proxy/http/remap/NextHopRoundRobin.h
@@ -33,12 +33,10 @@ class NextHopRoundRobin : public NextHopSelectionStrategy
public:
NextHopRoundRobin() = delete;
- NextHopRoundRobin(const std::string_view &name, const NHPolicyType &policy) : NextHopSelectionStrategy(name, policy) {}
- ~NextHopRoundRobin();
- bool
- Init(ts::Yaml::Map &n)
+ NextHopRoundRobin(const std::string_view &name, const NHPolicyType &policy, ts::Yaml::Map &n)
+ : NextHopSelectionStrategy(name, policy, n)
{
- return NextHopSelectionStrategy::Init(n);
}
+ ~NextHopRoundRobin();
void findNextHop(TSHttpTxn txnp, void *ih = nullptr, time_t now = 0) override;
};
diff --git a/proxy/http/remap/NextHopSelectionStrategy.cc b/proxy/http/remap/NextHopSelectionStrategy.cc
index 0ae5677f3..db96a48fc 100644
--- a/proxy/http/remap/NextHopSelectionStrategy.cc
+++ b/proxy/http/remap/NextHopSelectionStrategy.cc
@@ -41,21 +41,12 @@ constexpr std::string_view passive_health_check = "passive";
constexpr const char *policy_strings[] = {"NH_UNDEFINED", "NH_FIRST_LIVE", "NH_RR_STRICT",
"NH_RR_IP", "NH_RR_LATCHED", "NH_CONSISTENT_HASH"};
-NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, const NHPolicyType &policy)
+NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, const NHPolicyType &policy, ts::Yaml::Map &n)
+ : strategy_name(name), policy_type(policy)
{
- strategy_name = name;
- policy_type = policy;
-
+ NH_Debug(NH_DEBUG_TAG, "NextHopSelectionStrategy calling constructor");
NH_Debug(NH_DEBUG_TAG, "Using a selection strategy of type %s", policy_strings[policy]);
-}
-//
-// parse out the data for this strategy.
-//
-bool
-NextHopSelectionStrategy::Init(ts::Yaml::Map &n)
-{
- NH_Debug(NH_DEBUG_TAG, "calling Init()");
std::string self_host;
bool self_host_used = false;
@@ -235,29 +226,26 @@ NextHopSelectionStrategy::Init(ts::Yaml::Map &n)
throw std::invalid_argument("self host (" + self_host + ") does not appear in the first (peer) group");
}
} catch (std::exception &ex) {
- NH_Error("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
- return false;
+ throw std::invalid_argument("Error parsing the strategy named '" + strategy_name + "' due to '" + ex.what() +
+ "', this strategy will be ignored.");
}
if (ring_mode == NH_PEERING_RING) {
if (groups == 1) {
if (!go_direct) {
- NH_Error("when ring mode is '%s', go_direct must be true when there is only one host group.", peering_rings.data());
- return false;
+ throw std::invalid_argument("ring mode '" + std::string(peering_rings) +
+ "' go_direct must be true when there is only one host group");
}
} else if (groups != 2) {
- NH_Error("when ring mode is '%s', requires two host groups (peering group and an upstream group),"
- " or just a single peering group with go_direct.",
- peering_rings.data());
- return false;
+ throw std::invalid_argument(
+ "ring mode '" + std::string(peering_rings) +
+ "' requires two host groups (peering group and an upstream group), or a single peering group with go_direct");
}
if (policy_type != NH_CONSISTENT_HASH) {
- NH_Error("ring mode '%s', is only implemented for a 'consistent_hash' policy.", peering_rings.data());
- return false;
+ throw std::invalid_argument("ring mode '" + std::string(peering_rings) +
+ "' is only implemented for a 'consistent_hash' policy");
}
}
-
- return true;
}
void
diff --git a/proxy/http/remap/NextHopSelectionStrategy.h b/proxy/http/remap/NextHopSelectionStrategy.h
index 667b68e5b..54c0df5fc 100644
--- a/proxy/http/remap/NextHopSelectionStrategy.h
+++ b/proxy/http/remap/NextHopSelectionStrategy.h
@@ -193,10 +193,9 @@ private:
class NextHopSelectionStrategy
{
public:
- NextHopSelectionStrategy();
- NextHopSelectionStrategy(const std::string_view &name, const NHPolicyType &type);
+ NextHopSelectionStrategy() = delete;
+ NextHopSelectionStrategy(const std::string_view &name, const NHPolicyType &type, ts::Yaml::Map &n);
virtual ~NextHopSelectionStrategy(){};
- bool Init(ts::Yaml::Map &n);
virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr, time_t now = 0) = 0;
void markNextHop(TSHttpTxn txnp, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
const time_t now = 0);
diff --git a/proxy/http/remap/NextHopStrategyFactory.cc b/proxy/http/remap/NextHopStrategyFactory.cc
index 0db4a893c..ca1544a16 100644
--- a/proxy/http/remap/NextHopStrategyFactory.cc
+++ b/proxy/http/remap/NextHopStrategyFactory.cc
@@ -139,29 +139,25 @@ NextHopStrategyFactory::createStrategy(const std::string &name, const NHPolicyTy
return;
}
- switch (policy_type) {
- case NH_FIRST_LIVE:
- case NH_RR_STRICT:
- case NH_RR_IP:
- case NH_RR_LATCHED:
- strat_rr = std::make_shared<NextHopRoundRobin>(name, policy_type);
- if (strat_rr->Init(node)) {
+ try {
+ switch (policy_type) {
+ case NH_FIRST_LIVE:
+ case NH_RR_STRICT:
+ case NH_RR_IP:
+ case NH_RR_LATCHED:
+ strat_rr = std::make_shared<NextHopRoundRobin>(name, policy_type, node);
_strategies.emplace(std::make_pair(std::string(name), strat_rr));
- } else {
- strat.reset();
- }
- break;
- case NH_CONSISTENT_HASH:
- strat_chash = std::make_shared<NextHopConsistentHash>(name, policy_type);
- if (strat_chash->Init(node)) {
+ break;
+ case NH_CONSISTENT_HASH:
+ strat_chash = std::make_shared<NextHopConsistentHash>(name, policy_type, node);
_strategies.emplace(std::make_pair(std::string(name), strat_chash));
- } else {
- strat_chash.reset();
- }
- break;
- default: // handles P_UNDEFINED, no strategy is added
- break;
- };
+ break;
+ default: // handles P_UNDEFINED, no strategy is added
+ break;
+ };
+ } catch (std::exception &ex) {
+ strat.reset();
+ }
}
std::shared_ptr<NextHopSelectionStrategy>