Retry when safe #41
Merged
Retry when safe #41
Conversation
This file contains hidden or 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
Add __init__.py in tests/ folder so that nosetests can run individual tests. Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
This patch add the retrier system for all calls to the api (even unsafe ones). Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Right now the API does not raises 404 errors when we delete a resource that does not exists. Thus we can retry delete requests until we get a 200. A correct implementation would be to catch 404 errors as "delete success", which is not needed now and complexify the retry implementation (special retry case only for delete requests). A test `test_delete_on_hubstorage_api_does_not_404` will be triggered when the assumption does not hold anymore and the retry policy should be fixed. Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
| if (isinstance(err, ConnectionError) and err.args[0] == 'Connection aborted.' and | ||
| isinstance(err.args[1], BadStatusLine) and err.args[1][0] == repr('')): | ||
| logger.warning("Protocol failed with BadStatusLine, retrying (maybe)") | ||
| return True |
There was a problem hiding this comment.
Can this conditional run into an updated urllib3 (or httplib) that got fixed with Python 3.5 ? Do we need to handle RemoteDisconnected error, too?
There was a problem hiding this comment.
RemoteDisconnected inherits from BadStatusLine their should be no difference,
but actually, after testing the code in python3 it appears that it triggers a different structure for the same error. Catching it would look like:
isinstance(err, ConnectionError)
and isinstance(err.args[0], ProtocolError)
and err.args[0].args[0] == 'Connection aborted.'
and isinstance(err.args[0].args[1], BadStatusLine))
This is terrible, I'm looking at an idiomatic way to check the list of chained exception, something like:
if BadStatusLineError in all_chained_errors(err)
Thanks for the note!
Allows calls to apidelete to override the default is_idempotent behavior. Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
| @@ -14,6 +17,24 @@ | |||
| __version__ = pkgutil.get_data('hubstorage', 'VERSION').strip() | |||
|
|
|||
|
|
|||
| logger = logging.getLogger('HubstorageClient') | |||
|
|
|||
| _ERROR_CODES_TO_RETRY = (429, 503, 504) | |||
There was a problem hiding this comment.
For a global retry code list, I think we should add 408 too.
More details at http://blog.haproxy.com/2014/05/26/haproxy-and-http-errors-408-in-chrome/
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Fixes #14 examples (504 and timeouts) Signed-off-by: lsenta <laurent.senta@gmail.com>
Change where the dispatch occurs so that _iter_lines simply forward the request configuration to the client. The client decides whether to apply the retry policy depending on request args. Move the default GET behavior to apiget so that iter_json method is not altered by this change. (it already implements a retry that may be incompatible). Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
A client may define a max_retries and max_retry_time. Setting only max_retry=N means that the client will retry N times, no matter the time it takes. Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
Signed-off-by: lsenta <laurent.senta@gmail.com>
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.
Add a retry policy to idempotent requests (GET, DELETE, POST that sets data),
max_retriesto 3 so that users automatically benefit from this change.Add dependency to
responseslibrary for testing (mock therequestslibrary).