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

More HTTP status codes to retry on #4473

Merged
merged 6 commits into from
Oct 24, 2017
Merged

More HTTP status codes to retry on #4473

merged 6 commits into from
Oct 24, 2017

Conversation

vartec
Copy link
Contributor

@vartec vartec commented May 9, 2017

Add --retry-status <HTTP code> to allow specifying custom HTTP stauts codes to retry on. This is useful for AWS S3 or Cloudflare, which at times return codes other than 503 for intermittent failures.

Add --retry-status <HTTP code> to allow specifying custom HTTP stau…
307db7e 
…s codes to retry on.

This is useful for AWS S3 or Cloudflare, which at times return codes other than 503 for
itermitent failures.
@pradyunsg pradyunsg added type: enhancement Improvements to functionality state: needs discussion This needs some more discussion labels Jul 20, 2017
pradyunsg
pradyunsg reviewed Jul 20, 2017
View reviewed changes
setup.py Outdated
@@ -29,7 +29,7 @@ def find_version(*file_paths):
tests_require = [
'pytest',
'mock',
'pretend'
'pretend',
Copy link
Member

Choose a reason for hiding this comment

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

This has been fixed in master - could you merge master into this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pradyunsg done, I've merged the latest upstream master

@pradyunsg
Copy link
Member

LGTM except for the extra change which I believe has merged into master.

@dstufft
Copy link
Member

dstufft commented Jul 20, 2017

What do they return for intermittent failure?

@pradyunsg
Copy link
Member

Cloudflare expands the 5xx series with their own errors - https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#Cloudflare; I assumed that that's what this PR is for.

@vartec
Copy link
Contributor Author

vartec commented Jul 21, 2017

@dstufft for example S3 would at times return500, but that would actually mean just one failed node, subsequent attempts would most likely go to a healthy node an succeed.

@dstufft
Copy link
Member

dstufft commented Jul 21, 2017

My underlying question here is really: Can we just expand our default list to cover these cases to remove the need for an option, or are they going to be status codes where it would be a bad idea to retry in the general case.

If they're all 5xx errors, then I think we can probably just allow retries on all of those codes? Given this API is read only, retrying should be completely safe.

@vartec
Copy link
Contributor Author

vartec commented Jul 21, 2017

If they're all 5xx errors, then I think we can probably just allow retries on all of those codes? Given this API is read only, retrying should be completely safe.

I guess this depends how strictly you want to follow HTTP specs. The reason I'd see it as an option is that under normal circumstances you probably shouldn't retry on 500, but CDNs are kind of special case.

That being said, I would be fine with option --retry-on-all-errors or even making such behavior default w/o an option.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017
@xavfernandez
Copy link
Member

Like @dstufft I'd be in favor of adding some of those codes to our retry list rather than adding this (very specific) option.

@pradyunsg
Copy link
Member

@vartec It'd be awesome if you could make a new PR (or modify this one) to do what @dstufft and @xavfernandez are suggesting.

@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Oct 12, 2017
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 19, 2017
@vartec
Copy link
Contributor Author

vartec commented Oct 19, 2017

@pradyunsg, @dstufft, @xavfernandez – per your suggestions I've changed it to hardcoded statuses and removed an option

@vartec vartec changed the title Add --retry-status <HTTP code> to allow specifying custom HTTP status codes to retry on More HTTP status codes to retry on Oct 19, 2017
@pradyunsg pradyunsg removed S: awaiting response Waiting for a response/more information state: needs discussion This needs some more discussion labels Oct 19, 2017
@xavfernandez
Copy link
Member

xavfernandez commented Oct 23, 2017 •
edited
Loading

@vartec It looks like there is a non-ascii character in your comment, breaking the build in python 2

@xavfernandez xavfernandez added this to the 10.0 milestone Oct 23, 2017
@xavfernandez xavfernandez merged commit 62a695c into pypa:master Oct 24, 2017
@xavfernandez
Copy link
Member

@vartec thanks for the PR 👍

kianasun pushed a commit to kianasun/pip that referenced this pull request Mar 28, 2018
@vartec @kianasun
More HTTP status codes to retry on (pypa#4473)
32103ac 
* Add `--retry-status <HTTP code>` to allow specifying custom HTTP staus codes to retry on.
This is useful for AWS S3 or Cloudflare, which at times return codes other than 503 for
itermitent failures.

* stripping option related code, hardcoding the statuses

* Reword the news file

* changing ndash to regular ascii dash
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
6 participants
@vartec @pradyunsg @dstufft @BrownTruck @xavfernandez @pypa-bot