Change parent_select Init func to constructor

plugins/experimental/parent_select/consistenthash.cc

@@ -106,19 +106,17 @@ 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());
 }
 
 #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

plugins/experimental/parent_select/consistenthash.h

@@ -73,9 +73,8 @@ class PLNextHopConsistentHash : public PLNextHopSelectionStrategy
   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;

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.

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

plugins/experimental/parent_select/strategy.h

@@ -240,10 +240,9 @@ class TSNextHopSelectionStrategy
 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);
 
   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,

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

proxy/http/remap/NextHopConsistentHash.h

@@ -47,9 +47,9 @@ class NextHopConsistentHash : public NextHopSelectionStrategy
   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);

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;
 };

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

proxy/http/remap/NextHopSelectionStrategy.h

@@ -193,10 +193,9 @@ class NextHopHealthStatus : public NHHealthStatus
 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);

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>