Correctly retry HTTP downloads after incomplete reads#1578
Conversation
The [previous change] only helped urllib3 *notice* incomplete reads, but it could not retry given this was a streaming download. This commits implements the actual retries. [previous change]: elastic#1504
pquentin
added
bug
pquentin
changed the title
|
@elasticmachine run rally/rally-tracks-compat please |
dliappis
left a comment
dliappis
left a comment
There was a problem hiding this comment.
Looking forward to this stability improvement. Left a question.
| except urllib3.exceptions.ProtocolError as exc: | ||
| if i == HTTP_DOWNLOAD_RETRIES: | ||
| raise | ||
| logger.warning("Retrying after %s", exc) |
There was a problem hiding this comment.
Should we have a delay in between retries? Alternatively we could consider using something like https://pypi.org/project/tenacity/?
There was a problem hiding this comment.
I considered introducing a new dependency but thought this was not needed. I did not introduce a delay because I only saw connectivity issues (connection reset, incomplete reads) and not because the server was overloaded. Maybe a simple 1 second sleep could be enough?
There was a problem hiding this comment.
Agreed on a simple sleep; I'd go for 5s, but up to you.
There was a problem hiding this comment.
Done, please take another look.
|
|
||
| def download_http(url, local_path, expected_size_in_bytes=None, progress_indicator=None, *, sleep=time.sleep): | ||
| logger = logging.getLogger(__name__) | ||
| for i in range(HTTP_DOWNLOAD_RETRIES + 1): |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
For reference, the reason is that range(10) would only do 9 retries (1 try + 9 retries).
2 participants
The previous change only helped urllib3 notice incomplete reads, but it could not retry given this was a streaming download. This commits implements the actual retries.