WIP: address review comments.

Author: jrushford

doc/admin-guide/files/strategies.yaml.en.rst

@@ -184,12 +184,13 @@ Each **strategy** in the list may using the following parameters::
 - **parent_is_proxy**: A boolean value which indicates if the groups of hosts are proxy caches or origins.  **true** (default) means all the hosts used in the reamp are trafficserver caches.  **false** means the hosts are origins that the next hop strategies may use for load balancing and/or failover.
 - **scheme Indicates which scheme the strategy supports, *http* or *https*
 - **failover**: A map of **failover** information.
-- **max_simple_retries**: Part of the **failover** map and is an integer value of the maixmum number of retries for simple retry on the list of indicated response codes.
+- **max_simple_retries**: Part of the **failover** map and is an integer value of the maximum number of retries for a **simple retry** on the list of indicated response codes.  **simple retry** is used to retry an upstream request using another upstream server if the response received on from the original upstream request matches any of the response codes configured for this strategy in the **failover** map.  If no failover response codes are configured, no **simple retry** is attempted.
+
 - **ring_mode**: Part of the **failover** map. The host ring selection mode.  Use either **exhaust_ring** or **alternate_ring**
    #. **exhaust_ring**: when a host normally selected by the policy fails, another host is selected from the same group.  A new group is not selected until all hosts on the previous group have been exhausted.
    #. **alternate_ring**: retry hosts are selected from groups in an alternatin group fashion.
 - **response_codes**: Part of the **failover** map.  This is a list of **http** response codes that may be used for **simple retry**.
-- **health_check**: Part of the **failover** map.  A list of health checks. **passive** is the default and means that the state machine marks down **hosts** when a transaction timeout or connection error is detected.  **passive** is always used by the next hop strategies.  **active** means that some external process may actively health check the hosts using the defined **health check url** and mart them down using **traffic_ctl**.
+- **health_check**: Part of the **failover** map.  A list of health checks. **passive** is the default and means that the state machine marks down **hosts** when a transaction timeout or connection error is detected.  **passive** is always used by the next hop strategies.  **active** means that some external process may actively health check the hosts using the defined **health check url** and mark them down using **traffic_ctl**.
 
 
 Example::

proxy/http/HttpTransact.cc

@@ -207,8 +207,8 @@ findParent(HttpTransact::State *s)
   url_mapping *mp = s->url_map.getMapping();
 
   if (mp && mp->strategy) {
-    return mp->strategy->findNextHop(s->state_machine->sm_id, &s->parent_result, &s->request_data,
-                                     s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
+    return mp->strategy->findNextHop(s->state_machine->sm_id, s->parent_result, s->request_data, s->txn_conf->parent_fail_threshold,
+                                     s->txn_conf->parent_retry_time);
   } else if (s->parent_params) {
     return s->parent_params->findParent(&s->request_data, &s->parent_result, s->txn_conf->parent_fail_threshold,
                                         s->txn_conf->parent_retry_time);
@@ -223,7 +223,7 @@ markParentDown(HttpTransact::State *s)
   url_mapping *mp = s->url_map.getMapping();
 
   if (mp && mp->strategy) {
-    return mp->strategy->markNextHopDown(s->state_machine->sm_id, &s->parent_result, s->txn_conf->parent_fail_threshold,
+    return mp->strategy->markNextHopDown(s->state_machine->sm_id, s->parent_result, s->txn_conf->parent_fail_threshold,
                                          s->txn_conf->parent_retry_time);
   } else if (s->parent_params) {
     return s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
@@ -237,7 +237,7 @@ markParentUp(HttpTransact::State *s)
 {
   url_mapping *mp = s->url_map.getMapping();
   if (mp && mp->strategy) {
-    return mp->strategy->markNextHopUp(s->state_machine->sm_id, &s->parent_result);
+    return mp->strategy->markNextHopUp(s->state_machine->sm_id, s->parent_result);
   } else if (s->parent_params) {
     return s->parent_params->markParentUp(&s->parent_result);
   }
@@ -251,8 +251,8 @@ nextParent(HttpTransact::State *s)
   url_mapping *mp = s->url_map.getMapping();
   if (mp && mp->strategy) {
     // NextHop only has a findNextHop() function.
-    return mp->strategy->findNextHop(s->state_machine->sm_id, &s->parent_result, &s->request_data,
-                                     s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
+    return mp->strategy->findNextHop(s->state_machine->sm_id, s->parent_result, s->request_data, s->txn_conf->parent_fail_threshold,
+                                     s->txn_conf->parent_retry_time);
   } else if (s->parent_params) {
     return s->parent_params->nextParent(&s->request_data, &s->parent_result, s->txn_conf->parent_fail_threshold,
                                         s->txn_conf->parent_retry_time);

proxy/http/remap/NextHopConsistentHash.cc

@@ -97,7 +97,7 @@ NextHopConsistentHash::getHashKey(uint64_t sm_id, HttpRequestData *hrdata, ATSHa
     url_string_ref = url->string_get_ref(&len, true);
     if (url_string_ref && len > 0) {
       h->update(url_string_ref, len);
-      NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] url hash string: %s", sm_id, url_string_ref);
+      NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] url hash string: %s", sm_id, url_string_ref);
     }
     h->final();
     hash_value = h->get();
@@ -147,14 +147,14 @@ NextHopConsistentHash::getHashKey(uint64_t sm_id, HttpRequestData *hrdata, ATSHa
     if (ps_url) {
       url_string_ref = ps_url->string_get_ref(&len);
       if (url_string_ref && len > 0) {
-        NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] using parent selection over-ride string:'%.*s'.", sm_id, len, url_string_ref);
+        NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] using parent selection over-ride string:'%.*s'.", sm_id, len, url_string_ref);
         h->update(url_string_ref, len);
       }
     } else {
       url_string_ref = url->path_get(&len);
       h->update("/", 1);
       if (url_string_ref && len > 0) {
-        NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] the parent selection over-ride url is not set, using default path: %s.", sm_id,
+        NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] the parent selection over-ride url is not set, using default path: %s.", sm_id,
                  url_string_ref);
         h->update(url_string_ref, len);
       }
@@ -178,7 +178,7 @@ NextHopConsistentHash::getHashKey(uint64_t sm_id, HttpRequestData *hrdata, ATSHa
 }
 
 void
-NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, RequestData *rdata, const uint64_t fail_threshold,
+NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult &result, RequestData &rdata, const uint64_t fail_threshold,
                                    const uint64_t retry_time)
 {
   bool firstcall    = false;
@@ -189,43 +189,43 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
   uint64_t hash_key = 0;
   uint32_t lookups  = 0;
   ATSHash64Sip24 hash;
-  HttpRequestData *request_info    = static_cast<HttpRequestData *>(rdata);
+  HttpRequestData *request_info    = static_cast<HttpRequestData *>(&rdata);
   HostRecord *hostRec              = nullptr;
   std::shared_ptr<HostRecord> pRec = nullptr;
   HostStatus &pStatus              = HostStatus::instance();
   HostStatus_t host_stat           = HostStatus_t::HOST_STATUS_INIT;
   HostStatRec *hst                 = nullptr;
 
-  if (result->line_number == -1 && result->result == PARENT_UNDEFINED) {
+  if (result.line_number == -1 && result.result == PARENT_UNDEFINED) {
     firstcall = true;
   }
 
   if (firstcall) {
-    NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] firstcall, line_number: %d, result: %s", sm_id, result->line_number,
-             ParentResultStr[result->result]);
-    result->line_number = distance;
-    cur_ring            = 0;
+    NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] firstcall, line_number: %d, result: %s", sm_id, result.line_number,
+             ParentResultStr[result.result]);
+    result.line_number = distance;
+    cur_ring           = 0;
     for (uint32_t i = 0; i < groups; i++) {
-      result->chash_init[i] = false;
-      wrap_around[i]        = false;
+      result.chash_init[i] = false;
+      wrap_around[i]       = false;
     }
   } else {
-    NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] not firstcall, line_number: %d, result: %s", sm_id, result->line_number,
-             ParentResultStr[result->result]);
+    NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] not firstcall, line_number: %d, result: %s", sm_id, result.line_number,
+             ParentResultStr[result.result]);
     switch (ring_mode) {
     case NH_ALTERNATE_RING:
       if (groups > 1) {
-        cur_ring = (result->last_group + 1) % groups;
+        cur_ring = (result.last_group + 1) % groups;
       } else {
-        cur_ring = result->last_group;
+        cur_ring = result.last_group;
       }
       break;
     case NH_EXHAUST_RING:
     default:
       if (!wrapped) {
-        cur_ring = result->last_group;
+        cur_ring = result.last_group;
       } else if (groups > 1) {
-        cur_ring = (result->last_group + 1) % groups;
+        cur_ring = (result.last_group + 1) % groups;
       }
       break;
     }
@@ -236,23 +236,23 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
 
   do { // search until we've selected a different parent if !firstcall
     std::shared_ptr<ATSConsistentHash> r = rings[cur_ring];
-    hostRec = chash_lookup(r, hash_key, &result->chashIter[cur_ring], &wrapped, &hash, &result->chash_init[cur_ring], sm_id);
+    hostRec = chash_lookup(r, hash_key, &result.chashIter[cur_ring], &wrapped, &hash, &result.chash_init[cur_ring], sm_id);
     wrap_around[cur_ring] = wrapped;
     lookups++;
     // the 'available' flag is maintained in 'host_groups' and not the hash ring.
     if (hostRec) {
       pRec = host_groups[hostRec->group_index][hostRec->host_index];
       if (firstcall) {
-        hst                         = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
-        result->first_choice_status = (hst) ? hst->status : HostStatus_t::HOST_STATUS_UP;
+        hst                        = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
+        result.first_choice_status = (hst) ? hst->status : HostStatus_t::HOST_STATUS_UP;
         break;
       }
     } else {
       pRec = nullptr;
     }
-  } while (pRec && result->hostname && strcmp(pRec->hostname.c_str(), result->hostname) == 0);
+  } while (pRec && result.hostname && strcmp(pRec->hostname.c_str(), result.hostname) == 0);
 
-  NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] Initial parent lookups: %d", sm_id, lookups);
+  NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Initial parent lookups: %d", sm_id, lookups);
 
   // ----------------------------------------------------------------------------------------------------
   // Validate initial parent look-up and perform additional look-ups if required.
@@ -274,12 +274,12 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
         time_t now = time(nullptr);
         // check if the host is retryable.  It's retryable if the retry window has elapsed
         if ((pRec->failedAt + retry_time) < static_cast<unsigned>(now)) {
-          nextHopRetry        = true;
-          result->last_parent = pRec->host_index;
-          result->last_lookup = pRec->group_index;
-          result->retry       = nextHopRetry;
-          result->result      = PARENT_SPECIFIED;
-          NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] next hop %s is now retryable, marked it available.", sm_id, pRec->hostname.c_str());
+          nextHopRetry       = true;
+          result.last_parent = pRec->host_index;
+          result.last_lookup = pRec->group_index;
+          result.retry       = nextHopRetry;
+          result.result      = PARENT_SPECIFIED;
+          NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] next hop %s is now retryable, marked it available.", sm_id, pRec->hostname.c_str());
           break;
         }
       }
@@ -297,7 +297,7 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
         break;
       }
       std::shared_ptr<ATSConsistentHash> r = rings[cur_ring];
-      hostRec = chash_lookup(r, hash_key, &result->chashIter[cur_ring], &wrapped, &hash, &result->chash_init[cur_ring], sm_id);
+      hostRec = chash_lookup(r, hash_key, &result.chashIter[cur_ring], &wrapped, &hash, &result.chash_init[cur_ring], sm_id);
       wrap_around[cur_ring] = wrapped;
       lookups++;
       if (hostRec) {
@@ -311,7 +311,7 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
             host_stat = HOST_STATUS_UP;
           }
         }
-        NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] Selected a new parent: %s, available: %s, wrapped: %s, lookups: %d.", sm_id,
+        NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Selected a new parent: %s, available: %s, wrapped: %s, lookups: %d.", sm_id,
                  pRec->hostname.c_str(), (pRec->available) ? "true" : "false", (wrapped) ? "true" : "false", lookups);
         // use available host.
         if (pRec->available && host_stat == HOST_STATUS_UP) {
@@ -327,7 +327,7 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
         }
       }
       if (all_wrapped) {
-        NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] No available parents.", sm_id);
+        NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] No available parents.", sm_id);
         if (pRec) {
           pRec = nullptr;
         }
@@ -344,33 +344,33 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult *result, R
   hst       = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
   host_stat = (hst) ? hst->status : HostStatus_t::HOST_STATUS_UP;
 
-  if (pRec && host_stat == HOST_STATUS_UP && (pRec->available || result->retry)) {
-    result->result      = PARENT_SPECIFIED;
-    result->hostname    = pRec->hostname.c_str();
-    result->last_parent = pRec->host_index;
-    result->last_lookup = result->last_group = cur_ring;
+  if (pRec && host_stat == HOST_STATUS_UP && (pRec->available || result.retry)) {
+    result.result      = PARENT_SPECIFIED;
+    result.hostname    = pRec->hostname.c_str();
+    result.last_parent = pRec->host_index;
+    result.last_lookup = result.last_group = cur_ring;
     switch (scheme) {
     case NH_SCHEME_NONE:
     case NH_SCHEME_HTTP:
-      result->port = pRec->getPort(scheme);
+      result.port = pRec->getPort(scheme);
       break;
     case NH_SCHEME_HTTPS:
-      result->port = pRec->getPort(scheme);
+      result.port = pRec->getPort(scheme);
       break;
     }
-    result->retry = nextHopRetry;
-    ink_assert(result->hostname != nullptr);
-    ink_assert(result->port != 0);
-    NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] Chosen parent: %s.%d", sm_id, result->hostname, result->port);
+    result.retry = nextHopRetry;
+    ink_assert(result.hostname != nullptr);
+    ink_assert(result.port != 0);
+    NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Chosen parent: %s.%d", sm_id, result.hostname, result.port);
   } else {
     if (go_direct == true) {
-      result->result = PARENT_DIRECT;
+      result.result = PARENT_DIRECT;
     } else {
-      result->result = PARENT_FAIL;
+      result.result = PARENT_FAIL;
     }
-    result->hostname = nullptr;
-    result->port     = 0;
-    result->retry    = false;
+    result.hostname = nullptr;
+    result.port     = 0;
+    result.retry    = false;
   }
 
   return;

proxy/http/remap/NextHopConsistentHash.h

@@ -38,6 +38,6 @@ class NextHopConsistentHash : public NextHopSelectionStrategy
   NextHopConsistentHash(const std::string_view name, const NHPolicyType &policy) : NextHopSelectionStrategy(name, policy) {}
   ~NextHopConsistentHash();
   bool Init(const YAML::Node &n);
-  void findNextHop(const uint64_t sm_id, ParentResult *result, RequestData *rdata, const uint64_t fail_threshold,
+  void findNextHop(const uint64_t sm_id, ParentResult &result, RequestData &rdata, const uint64_t fail_threshold,
                    const uint64_t retry_time) override;
 };