[Discussion] Don’t retry response status codes from handle_httpstatus_list #3675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3675 +/- ##
=======================================
Coverage 84.52% 84.52%
=======================================
Files 167 167
Lines 9410 9410
Branches 1397 1397
=======================================
Hits 7954 7954
Misses 1199 1199
Partials 257 257
|
lucywang000
reviewed
Mar 9, 2019
| if (request.meta.get('dont_retry', False) or | ||
| response.status in getattr(spider, 'handle_httpstatus_list', []) or | ||
| response.status in request.meta.get('handle_httpstatus_list', []) or | ||
| request.meta.get('handle_httpstatus_all', False)): |
There was a problem hiding this comment.
We could keep it back compatible by only taking handle_httpstatus_all into consider when there is a settings key, for instance "DONT_RETRY_ HANDLE_HTTPSTATUS_ALL = True"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Given a request with
'handle_httpstatus_list': [404]in a project withRETRY_HTTP_CODES = […, 404, …], there is currently no way to prevent retries of that specific request only for 404 responses.These changes solve that issue by copying some code from the Redirect middleware into the Retry middleware, making the latter take
handle_httpstatus_listandhandle_httpstatus_allinto account for retries.These changes are backward incompatible. Not only do they change the current behavior, but they also make the previous behavior impossible without replacing the built-in Redirect middleware.
What are your thoughts? Should I go ahead and complete the pull request as is (add docs and tests)? Should we follow a backward-compatible approach, such as supporting a new
dont_retry_httpstatus_list? Can you think of a better approach? Or is it better for people to rewrite the Redirect middleware when they need this behavior?