-
Notifications
You must be signed in to change notification settings - Fork 48
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.
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
fix: add retries for 'requests.ConnectionError' #186
Conversation
Is it possible that this would also retry requests that have Seeing this in google-resumable-media==1.10 (python 2.7, but I imagine other versions are impacted):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just have nits on commentary.
google/resumable_media/_helpers.py
Outdated
def _get_connection_error_class(): | ||
"""Get the exception error class. | ||
|
||
This is a separate function for testing purposes.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify "testing purposes" with detecting if requests is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, good to explain this layer of indirection.
|
||
# Set the retriable_exception_type if possible. We expect requests to be | ||
# present here and the transport to be using requests.exceptions errors, | ||
# but due to loose coupling with the transport layer we can't guarantee it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you file an issue to track potentially improving this in the future? E.g. Reliance on having a specific transport requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
google/resumable_media/_helpers.py
Outdated
def _get_connection_error_class(): | ||
"""Get the exception error class. | ||
|
||
This is a separate function for testing purposes.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, good to explain this layer of indirection.
Feedback addressed, have two other reviews. Thanks!
Thanks for the work on this and for adding retries in the case of We're seeing quite a few Are there regularly scheduled releases of this package? Or is there another way we can get a sense for when this change might be released? |
@tschaub Working on a release today. |
Correction, it's Friday so we'll target Monday. |
Restructures retry strategy to check both status codes and exceptions; then, adds requests.ConnectionError as a retriable exception. (Note: the unified standard library ConnectionError class is not used because it is unsupported by both Requests and Python 2).