From 11cf3f097fa7d9f10c0131791249c56aab6839a6 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Thu, 18 May 2023 08:36:56 -0400 Subject: [PATCH] 12684: Stop retrying 422 responses in PySDK The original motivation for this was to retry when the API server was having database connectivity problems. The feeling eight years later is that things have changed enough that, on balance, this isn't worth retrying anymore. I don't think this will have any real impact on current Arvados software. In the main branch as I write this, `check_http_response_status` only gets called in five places. Three of those are in the main `arvados` module for job and task utilities, which presumably nobody is using anymore. The other two talk to Keep, which only returns 422 for hash mismatches, where a retry will definitely never succeed. Arvados-DCO-1.1-Signed-off-by: Brett Smith --- sdk/python/arvados/retry.py | 6 +----- sdk/python/tests/test_api.py | 4 ++-- sdk/python/tests/test_retry.py | 4 ++-- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/sdk/python/arvados/retry.py b/sdk/python/arvados/retry.py index e93624a5d1..2168146a4b 100644 --- a/sdk/python/arvados/retry.py +++ b/sdk/python/arvados/retry.py @@ -27,7 +27,7 @@ from collections import deque import arvados.errors _HTTP_SUCCESSES = set(range(200, 300)) -_HTTP_CAN_RETRY = set([408, 409, 422, 423, 500, 502, 503, 504]) +_HTTP_CAN_RETRY = set([408, 409, 423, 500, 502, 503, 504]) class RetryLoop(object): """Coordinate limited retries of code. @@ -200,10 +200,6 @@ def check_http_response_success(status_code): * Any 2xx result returns `True`. * A select few status codes, or any malformed responses, return `None`. - 422 Unprocessable Entity is in this category. This may not meet the - letter of the HTTP specification, but the Arvados API server will - use it for various server-side problems like database connection - errors. * Everything else returns `False`. Note that this includes 1xx and 3xx status codes. They don't indicate success, and you can't diff --git a/sdk/python/tests/test_api.py b/sdk/python/tests/test_api.py index d8136f4ac5..e54f387b39 100644 --- a/sdk/python/tests/test_api.py +++ b/sdk/python/tests/test_api.py @@ -38,7 +38,7 @@ if not mimetypes.inited: class ArvadosApiTest(run_test_server.TestCaseWithServers): MAIN_SERVER = {} ERROR_HEADERS = {'Content-Type': mimetypes.types_map['.json']} - RETRIED_4XX = frozenset([408, 409, 422, 423]) + RETRIED_4XX = frozenset([408, 409, 423]) def api_error_response(self, code, *errors): return (fake_httplib2_response(code, **self.ERROR_HEADERS), @@ -168,7 +168,7 @@ class ArvadosApiTest(run_test_server.TestCaseWithServers): def test_4xx_not_retried(self): client = arvados.api('v1', num_retries=3) - for code in [400, 401, 404]: + for code in [400, 401, 404, 422]: with self.subTest(f'error {code}'), mock.patch('time.sleep'): with mock_api_responses( client, diff --git a/sdk/python/tests/test_retry.py b/sdk/python/tests/test_retry.py index 2d02005937..bcf784d130 100644 --- a/sdk/python/tests/test_retry.py +++ b/sdk/python/tests/test_retry.py @@ -174,14 +174,14 @@ class CheckHTTPResponseSuccessTestCase(unittest.TestCase): self.check_is(True, *list(range(200, 207))) def test_obvious_stops(self): - self.check_is(False, 424, 426, 428, 431, + self.check_is(False, 422, 424, 426, 428, 431, *list(range(400, 408)) + list(range(410, 420))) def test_obvious_retries(self): self.check_is(None, 500, 502, 503, 504) def test_4xx_retries(self): - self.check_is(None, 408, 409, 422, 423) + self.check_is(None, 408, 409, 423) def test_5xx_failures(self): self.check_is(False, 501, *list(range(505, 512))) -- 2.30.2