Add retries with backoff and increase timeout in statistics_poland download script - #2157
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces robust HTTP session management with automatic retries and exponential backoff for downloading data from Statistics Poland. It adds helper functions get_http_session and make_request and refactors fetch_variables and the main download loop to use this session. The review feedback points out a redundant double-retry loop in make_request because the session is already configured with an HTTPAdapter and a Retry strategy. It is recommended to simplify make_request to rely entirely on the session's built-in retry strategy and increase the retry count to guarantee download success.
| def make_request(session, url, headers=None, params=None, timeout=60, max_attempts=5): | ||
| """Makes an HTTP GET request with exponential backoff on timeouts/connection errors.""" | ||
| for attempt in range(1, max_attempts + 1): | ||
| try: | ||
| resp = session.get(url, headers=headers, params=params, timeout=timeout) | ||
| return resp | ||
| except (requests.exceptions.Timeout, requests.exceptions.ConnectionError, requests.exceptions.RequestException) as e: | ||
| logging.warning(f"Request attempt {attempt}/{max_attempts} failed for {url}: {e}") | ||
| if attempt == max_attempts: | ||
| raise | ||
| time.sleep(2 ** attempt) | ||
| return None |
There was a problem hiding this comment.
The current implementation of make_request introduces a redundant double-retry loop. Since the requests.Session is already configured with an HTTPAdapter and a Retry strategy, wrapping session.get in a manual retry loop will cause nested retries. We should simplify make_request to rely entirely on the session's built-in retry strategy. Additionally, when downloading data from unstable or unreliable external sources like Statistics Poland, ensure that the session's retry strategy is configured with a higher number of retries (e.g., 10 retries) to guarantee download success, even if it results in a long cumulative wait time.
def make_request(session, url, headers=None, params=None, timeout=60):
# Ensure the session's retry strategy is configured with a high number of retries (e.g., 10)
return session.get(url, headers=headers, params=params, timeout=timeout)References
- When downloading data from unstable or unreliable external sources, configure a higher number of retries (e.g., tries=10) to ensure download success, even if it results in a long cumulative wait time.
There was a problem hiding this comment.
Addressed in commit f905d4c. Simplified make_request to rely directly on the session's built-in HTTPAdapter with urllib3.util.Retry and configured 10 retries with exponential backoff on all status codes and timeouts.
Description
Fixes download script failure in
statvar_imports/statistics_poland/download_input_data.py.Root Cause
https://bdl.stat.gov.pl/api/v1/) intermittently take > 20s, causingRead timed outfailures whentimeout=20.HTTP 429 Too Many Requests. The previous logic dropped non-200 responses without retrying.Fix
urllib3.util.Retryonrequests.Sessionfor connection drops and server 5xx errors.make_request): Added 5 retries with exponential backoff on timeouts/connection errors andHTTP 429/5xxstatus codes (respectingRetry-Afterheader).Dev Cloud Batch Verification (
us-west1)statistics-poland-abhishekjaisw-20260813-130859(UID:statistics-poland-5ae3a2da-5e3c-45c700)SUCCESS(Stage:STAGING)check_deleted_records_percentcheck_empty_importcheck_missing_refs_countcheck_lint_error_countcheck_goldens_summary_reportcheck_goldens_observations