From dc52e8f550a6097c05319f3376b9da460db05d14 Mon Sep 17 00:00:00 2001 From: Evan Zelkowitz Date: Tue, 2 Nov 2021 13:46:07 -0600 Subject: [PATCH 1/3] Add 5xx's to be allowed to be used for simple retries Remove unnecessary functions in transact for finding ranges Change PS response checking to not use internal state. Now pass in retry type and code Add note in retries about simple retry precedence --- doc/admin-guide/files/parent.config.en.rst | 12 ++++++-- proxy/ParentSelection.cc | 6 ++-- proxy/ParentSelection.h | 10 +++---- proxy/http/HttpTransact.cc | 34 +++++----------------- 4 files changed, 25 insertions(+), 37 deletions(-) diff --git a/doc/admin-guide/files/parent.config.en.rst b/doc/admin-guide/files/parent.config.en.rst index 39d33e36543..c1dfe93e032 100644 --- a/doc/admin-guide/files/parent.config.en.rst +++ b/doc/admin-guide/files/parent.config.en.rst @@ -214,20 +214,26 @@ The following list shows the possible actions and their allowed values. ``parent_retry`` - ``simple_retry`` - If the parent returns a 404 response or if the response matches - a list of http 4xx responses defined in ``simple_server_retry_responses`` on a request + a list of http 4xx and/or 5xx responses defined in ``simple_server_retry_responses`` on a request a new parent is selected and the request is retried. The number of retries is controlled by ``max_simple_retries`` which is set to 1 by default. - ``unavailable_server_retry`` - If the parent returns a 503 response or if the response matches a list of http 5xx responses defined in ``unavailable_server_retry_responses``, the currently selected parent is marked down and a new parent is selected to retry the request. The number of retries is controlled by ``max_unavailable_server_retries`` which is set to 1 by default. - - ``both`` - This enables both ``simple_retry`` and ``unavailable_server_retry`` as described above. + - ``both`` - This enables both ``simple_retry`` and ``unavailable_server_retry`` as described above. + + .. Note:: + + If a response code exists in both the simple and unavailable lists and both + is the retry type then simple_retry will take precedence and unavailable_server_retry + will not be used for that code. .. _parent-config-format-simple-server-retry-responses: ``simple_server_retry_responses`` If ``parent_retry`` is set to either ``simple_retry`` or ``both``, this parameter is a comma separated list of - http 4xx response codes that will invoke the ``simple_retry`` described in the ``parent_retry`` section. By + http 4xx and/or 5xx response codes that will invoke the ``simple_retry`` described in the ``parent_retry`` section. By default, ``simple_server_retry_responses`` is set to 404. .. _parent-config-format-unavailable-server-retry-responses: diff --git a/proxy/ParentSelection.cc b/proxy/ParentSelection.cc index 29f5bdf7892..48de7ab548a 100644 --- a/proxy/ParentSelection.cc +++ b/proxy/ParentSelection.cc @@ -361,17 +361,17 @@ SimpleRetryResponseCodes::SimpleRetryResponseCodes(char *val) numTok = pTok.Initialize(val, SHARE_TOKS); if (numTok == 0) { c = atoi(val); - if (c > 399 && c < 500) { + if (c > 399 && c < 600) { codes.push_back(HTTP_STATUS_NOT_FOUND); } } for (int i = 0; i < numTok; i++) { c = atoi(pTok[i]); - if (c > 399 && c < 500) { + if (c > 399 && c < 600) { Debug("parent_select", "loading simple response code: %d", c); codes.push_back(c); } else { - Warning("SimpleRetryResponseCodes received non-4xx code '%s', ignoring!", pTok[i]); + Warning("SimpleRetryResponseCodes received non-4xx or 5xx code '%s', ignoring!", pTok[i]); } } std::sort(codes.begin(), codes.end()); diff --git a/proxy/ParentSelection.h b/proxy/ParentSelection.h index 954e19cdb85..8b7748f3ea7 100644 --- a/proxy/ParentSelection.h +++ b/proxy/ParentSelection.h @@ -324,17 +324,17 @@ struct ParentResult { } bool - response_is_retryable(HTTPStatus response_code) const + response_is_retryable(ParentRetry_t retry_type, HTTPStatus response_code) const { - Debug("parent_select", "In response_is_retryable, code: %d", response_code); - if (retry_type() == PARENT_RETRY_BOTH) { + Debug("parent_select", "In response_is_retryable, code: %d, type: %d", response_code, retry_type); + if (retry_type == PARENT_RETRY_BOTH) { Debug("parent_select", "Saw retry both"); return (rec->unavailable_server_retry_responses->contains(response_code) || rec->simple_server_retry_responses->contains(response_code)); - } else if (retry_type() == PARENT_RETRY_UNAVAILABLE_SERVER) { + } else if (retry_type == PARENT_RETRY_UNAVAILABLE_SERVER) { Debug("parent_select", "Saw retry unavailable server"); return rec->unavailable_server_retry_responses->contains(response_code); - } else if (retry_type() == PARENT_RETRY_SIMPLE) { + } else if (retry_type == PARENT_RETRY_SIMPLE) { Debug("parent_select", "Saw retry simple retry"); return rec->simple_server_retry_responses->contains(response_code); } else { diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index b1f161d7fba..b35d619f8e7 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -304,26 +304,6 @@ is_localhost(const char *name, int len) return (len == (sizeof(local) - 1)) && (memcmp(name, local, len) == 0); } -inline static bool -is_response_simple_code(HTTPStatus response_code) -{ - if (static_cast(response_code) < 400 || static_cast(response_code) > 499) { - return false; - } - - return true; -} - -inline static bool -is_response_unavailable_code(HTTPStatus response_code) -{ - if (static_cast(response_code) < 500 || static_cast(response_code) > 599) { - return false; - } - - return true; -} - bool HttpTransact::is_response_valid(State *s, HTTPHdr *incoming_response) { @@ -399,20 +379,22 @@ response_is_retryable(HttpTransact::State *s, HTTPStatus response_code) return mp->strategy->responseIsRetryable(s->state_machine->sm_id, s->current, response_code); } - if (s->parent_params && !s->parent_result.response_is_retryable(response_code)) { + if (s->parent_params && !s->parent_result.response_is_retryable((ParentRetry_t)(s->parent_result.retry_type()), response_code)) { return PARENT_RETRY_NONE; } - - const unsigned int s_retry_type = retry_type(s); - const HTTPStatus server_response = http_hdr_status_get(s->hdr_info.server_response.m_http); - if ((s_retry_type & PARENT_RETRY_SIMPLE) && is_response_simple_code(server_response) && + const unsigned int s_retry_type = retry_type(s); + // If simple or both, check if code is simple-retryable and for retry attempts + if ((s_retry_type & PARENT_RETRY_SIMPLE) && s->parent_result.response_is_retryable(PARENT_RETRY_SIMPLE, response_code) && s->current.simple_retry_attempts < max_retries(s, PARENT_RETRY_SIMPLE)) { if (s->current.simple_retry_attempts < numParents(s)) { return PARENT_RETRY_SIMPLE; } return PARENT_RETRY_NONE; } - if ((s_retry_type & PARENT_RETRY_UNAVAILABLE_SERVER) && is_response_unavailable_code(server_response) && + // If unavailable or both, check if code is unavailable-retryable AND also not simple-retryable, then unavailable retry attempts + if ((s_retry_type & PARENT_RETRY_UNAVAILABLE_SERVER) && + s->parent_result.response_is_retryable(PARENT_RETRY_UNAVAILABLE_SERVER, response_code) && + !s->parent_result.response_is_retryable(PARENT_RETRY_SIMPLE, response_code) && s->current.unavailable_server_retry_attempts < max_retries(s, PARENT_RETRY_UNAVAILABLE_SERVER)) { if (s->current.unavailable_server_retry_attempts < numParents(s)) { return PARENT_RETRY_UNAVAILABLE_SERVER; From cec155bf44291422a4b7aff908f9969fcca49c1b Mon Sep 17 00:00:00 2001 From: Evan Zelkowitz Date: Mon, 15 Nov 2021 14:25:00 -0700 Subject: [PATCH 2/3] Fix clang format --- proxy/http/HttpTransact.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index b35d619f8e7..4f6a5233da5 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -386,6 +386,7 @@ response_is_retryable(HttpTransact::State *s, HTTPStatus response_code) // If simple or both, check if code is simple-retryable and for retry attempts if ((s_retry_type & PARENT_RETRY_SIMPLE) && s->parent_result.response_is_retryable(PARENT_RETRY_SIMPLE, response_code) && s->current.simple_retry_attempts < max_retries(s, PARENT_RETRY_SIMPLE)) { + TxnDebug("http_trans", "saw parent retry simple first in trans"); if (s->current.simple_retry_attempts < numParents(s)) { return PARENT_RETRY_SIMPLE; } @@ -396,6 +397,7 @@ response_is_retryable(HttpTransact::State *s, HTTPStatus response_code) s->parent_result.response_is_retryable(PARENT_RETRY_UNAVAILABLE_SERVER, response_code) && !s->parent_result.response_is_retryable(PARENT_RETRY_SIMPLE, response_code) && s->current.unavailable_server_retry_attempts < max_retries(s, PARENT_RETRY_UNAVAILABLE_SERVER)) { + TxnDebug("http_trans", "saw parent retry unavailable first in trans"); if (s->current.unavailable_server_retry_attempts < numParents(s)) { return PARENT_RETRY_UNAVAILABLE_SERVER; } From 82d7663222f6842da3211c1c38801c624bb35692 Mon Sep 17 00:00:00 2001 From: Evan Zelkowitz Date: Mon, 15 Nov 2021 14:30:49 -0700 Subject: [PATCH 3/3] Remove trailing whitespace in docs --- doc/admin-guide/files/parent.config.en.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/admin-guide/files/parent.config.en.rst b/doc/admin-guide/files/parent.config.en.rst index c1dfe93e032..50fa96fe737 100644 --- a/doc/admin-guide/files/parent.config.en.rst +++ b/doc/admin-guide/files/parent.config.en.rst @@ -221,12 +221,12 @@ The following list shows the possible actions and their allowed values. a list of http 5xx responses defined in ``unavailable_server_retry_responses``, the currently selected parent is marked down and a new parent is selected to retry the request. The number of retries is controlled by ``max_unavailable_server_retries`` which is set to 1 by default. - - ``both`` - This enables both ``simple_retry`` and ``unavailable_server_retry`` as described above. + - ``both`` - This enables both ``simple_retry`` and ``unavailable_server_retry`` as described above. .. Note:: - If a response code exists in both the simple and unavailable lists and both - is the retry type then simple_retry will take precedence and unavailable_server_retry + If a response code exists in both the simple and unavailable lists and both + is the retry type then simple_retry will take precedence and unavailable_server_retry will not be used for that code. .. _parent-config-format-simple-server-retry-responses: