masaori335 commented on code in PR #13102:
URL: https://github.com/apache/trafficserver/pull/13102#discussion_r3165810099
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3970,63 +4007,92 @@ HttpTransact::handle_response_from_server(State *s)
}
}
- if (is_server_negative_cached(s)) {
- max_connect_retries =
s->txn_conf->connect_attempts_max_retries_down_server - 1;
- } else {
- // server not yet negative cached - use default number of retries
- max_connect_retries = s->txn_conf->connect_attempts_max_retries;
- }
-
+ unsigned max_connect_retries = s->txn_conf->connect_attempts_max_retries;
TxnDbg(dbg_ctl_http_trans, "max_connect_retries: %d
s->current.retry_attempts: %d", max_connect_retries,
s->current.retry_attempts.get());
- if (is_request_retryable(s) && s->current.retry_attempts.get() <
max_connect_retries &&
- !HttpTransact::is_response_valid(s, &s->hdr_info.server_response)) {
- // If this is a round robin DNS entry & we're tried configured
- // number of times, we should try another node
- if (ResolveInfo::OS_Addr::TRY_CLIENT == s->dns_info.os_addr_style) {
- // attempt was based on client supplied server address. Try again
using HostDB.
- // Allow DNS attempt
- s->dns_info.resolved_p = false;
- // See if we can get data from HostDB for this.
- s->dns_info.os_addr_style = ResolveInfo::OS_Addr::TRY_HOSTDB;
- // Force host resolution to have the same family as the client.
- // Because this is a transparent connection, we can't switch address
- // families - that is locked in by the client source address.
- ats_force_order_by_family(s->current.server->dst_addr.family(),
s->my_txn_conf().host_res_data.order);
- return CallOSDNSLookup(s);
- } else if (ResolveInfo::OS_Addr::USE_API == s->dns_info.os_addr_style &&
!s->api_server_addr_set_retried) {
- // Plugin set the server address via TSHttpTxnServerAddrSet(). Clear
resolution
- // state to allow the OS_DNS hook to be called again, giving the
plugin a chance
- // to set a different server address for retry (issue #12611).
- // Only retry once to avoid infinite loops if the plugin keeps setting
failing addresses.
- s->api_server_addr_set_retried = true;
- s->dns_info.resolved_p = false;
- s->dns_info.os_addr_style = ResolveInfo::OS_Addr::TRY_DEFAULT;
- // Clear the server request so it can be rebuilt for the new
destination
- s->hdr_info.server_request.destroy();
- TxnDbg(dbg_ctl_http_trans, "Retrying with plugin-set address,
returning to OS_DNS hook");
- return CallOSDNSLookup(s);
- } else {
- if ((s->txn_conf->connect_attempts_rr_retries > 0) &&
- ((s->current.retry_attempts.get() + 1) %
s->txn_conf->connect_attempts_rr_retries == 0)) {
- s->dns_info.select_next_rr();
- }
- retry_server_connection_not_open(s, s->current.state,
max_connect_retries);
- TxnDbg(dbg_ctl_http_trans, "Error. Retrying...");
- s->next_action = how_to_open_connection(s);
- }
- } else {
+ // Bail out if the request is not retryable, the global retry cap is
reached, or we already have a usable response.
+ if (!is_request_retryable(s) || s->current.retry_attempts.get() >=
max_connect_retries ||
+ HttpTransact::is_response_valid(s, &s->hdr_info.server_response)) {
+ TxnDbg(dbg_ctl_http_trans, "Error. No more retries. %d/%d",
s->current.retry_attempts.get(), max_connect_retries);
Review Comment:
Here, we need to use `connect_attempts_max_retries unconditionally ` to give
a chance to round-robin case which can be new HostDBInfo in UP state. The
state-aware limit is set by line 4077 below.
```
// The active target (HostDB) may be SUSPECT state, so re-evaluate the
retry limit.
max_connect_retries = origin_server_connect_attempts_max_retries(s);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
