Repository navigation
Conversation
HTTPAdapter(max_retries=3) retries connection and read errors but not error responses (apart from 413/429/503 with a Retry-After header), so a single 503 from the API failed the request, and with it the processing job that made it. Retry GET and HEAD requests that get a 429/500/502/503/504 response up to 5 times with exponential backoff and jitter, and log a warning each time an error response is retried. Only GET and HEAD are retried after an error response or a read error, because resending an upload or a reprocess request has side effects; an upload PUT is no longer resent after a read error, such as the connection closing before the response arrives. raise_on_status=False keeps the "<status> <reason>: <body>" error once the retries run out. backoff_jitter needs urllib3 2.0, which is now a declared dependency.
|
Whelp. The red CI here isn't from this change. GitHub is moving Once we collectively decide on the right fix, I'll update this branch so CI reruns. |
tmplummer
left a comment
tmplummer
left a comment
There was a problem hiding this comment.
This is definitely outside my wheel house but the changes look fine to me.
When an upload's PUT loses its connection before S3 responds, urllib3 resends it today. IMAP-Science-Operations-Center/imap-data-access#373 stops that resend, since resending an upload blindly has side effects, so with that change alone, the "Connection aborted" error would reach upload_products and fail the job. Retry one level up instead: treat that error like a 503 SlowDown and run the whole upload again, which first checks whether S3 already has the file (409 FileAlreadyExists), and exit with RETRY_EXIT_CODE if it keeps failing.
|
IMAP-Science-Operations-Center/imap_processing#3626 has merged, so imap_processing's Added Bryan and Luisa as reviewers in case this is more in their wheelhouse. Open to suggestions if anyone else would be a better reviewer. |
|
Tests are passing again now that we've dropped Python 3.9 |
Change Summary
Overview
Closes #372.
Uses the
Retryproposed in the issue, so a GET or HEAD that gets a 429 or 5xx response is retried with exponential backoff and jitter instead of failing. That covers every request made through_make_request, includingspice_query,release, and the webpoda queries. The one addition is_LoggingRetry, a smallRetrysubclass that logs each retried error response as a warning, like the issue asks for; a plainRetrywould log those only at debug level, so job logs wouldn't show them otherwise.Worth knowing:
releaseis a GET, so it's retried too. A retry after a release that committed but still returned an error would also release any version that arrived in between.urllib3>=2.0, whichbackoff_jitterneeds. imap_processing and sds-data-manager already lock urllib3 2.7.0.File changes
imap_data_access/io.py: the retry policy and_LoggingRetry.pyproject.toml:urllib3>=2.0.tests/test_io.py: 8 tests that run the real retry loop, mocking only urllib3's connection pool.Testing
All 159 tests pass, including at the urllib3 2.0 floor. The new retry tests fail against the old adapter.