[GitHub] [trafficserver] rob05c commented on a diff in pull request #8831: Fix parent_select optional scheme

[GitBox <gi...@apache.org>]

rob05c commented on code in PR #8831:
URL: https://github.com/apache/trafficserver/pull/8831#discussion_r870517057


##########
plugins/experimental/parent_select/strategy.cc:
##########
@@ -71,16 +71,15 @@ PLNextHopSelectionStrategy::Init(const YAML::Node &n)
   bool self_host_used = false;
 
   try {
+    // scheme is optional, and strategies with no scheme will match hosts with no scheme
     if (n["scheme"]) {
       auto scheme_val = n["scheme"].Scalar();
       if (scheme_val == "http") {
         scheme = PL_NH_SCHEME_HTTP;
       } else if (scheme_val == "https") {
         scheme = PL_NH_SCHEME_HTTPS;
       } else {
-        scheme = PL_NH_SCHEME_NONE;
-        PL_NH_Note("Invalid 'scheme' value, '%s', for the strategy named '%s', setting to PL_NH_SCHEME_NONE", scheme_val.c_str(),
-                   strategy_name.c_str());
+        PL_NH_Note("Invalid scheme '%s' for strategy '%s', setting to NONE", scheme_val.c_str(), strategy_name.c_str());

Review Comment:
   I didn't write the func, it's inherited from core. But it has to exist because the object is constructed by the YAML library.
   
   I think `Init` is effectively like a constructor, because we can't construct, since we can't prevent the YAML library from doing the construction.
   
   But, I think `Init` does make that assumption. There are probably other data it doesn't reset either. I don't object to making Init safe in case someone re-init's an object, and resetting all members to their defaults, but that's probably a larger change and might be better as its own PR



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org