Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add REQUEST_TIMEOUT 408 as retryable code #270

Merged
merged 4 commits into from Sep 13, 2021
Merged

fix: add REQUEST_TIMEOUT 408 as retryable code #270

merged 4 commits into from Sep 13, 2021

Conversation

cojenco
Copy link
Contributor

@cojenco cojenco commented Sep 10, 2021 •
edited

This adds http.client.REQUEST_TIMEOUT (408) to retryable codes.

Per team discussion with the GCS FnD team, HTTP status code 408's Request Timeout can be handled with exponential backoff and is retryable. An uploadBrokenConnection will cause a 408. 408 is currently included in retryable codes in python-storage, and should also be included in the resumable media package.

Fixes #271

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/google-resumable-media-python API. label Sep 10, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 10, 2021
@cojenco cojenco marked this pull request as ready for review September 10, 2021 23:35
@cojenco cojenco requested review from a team as code owners September 10, 2021 23:35
tseaver
tseaver suggested changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to add http.client.REQUEST_TIMEOUT is fine. However, the changes to the tests are actually unrelated to that change, and unnecessary -- please revert them.

@cojenco
Copy link
Contributor Author

cojenco commented Sep 13, 2021

For test_success_with_retry, I've changed to retry all of the common.RETRYABLE codes. Thus we can ensure that all retryable codes are successfully retried in the mocks. Reverted the 2 other unnecessary changes. PTAL, thanks!

tseaver
tseaver suggested changes Sep 13, 2021
View reviewed changes
tests/unit/test__helpers.py Outdated Show resolved Hide resolved
tests/unit/test__helpers.py Outdated Show resolved Hide resolved
@cojenco cojenco merged commit d0ad0aa into googleapis:main Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver approved these changes

@tritone tritone tritone approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/google-resumable-media-python API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Retryable codes missing 408 Request Timeout
3 participants
@cojenco @tseaver @tritone