-
Notifications
You must be signed in to change notification settings - Fork 202
SG-35165 Retry on URLError #342
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
SG-35165 Retry on URLError #342
Conversation
carlos-villavicencio-adsk
commented
May 22, 2024
- Add retry only when encountering an URLError
- Adopt the existing strategy
- Add tests
# response headers are in str(resp.info()).splitlines() | ||
except urllib.error.URLError as e: | ||
LOG.debug("Got a %s response. Waiting and retrying..." % e) | ||
time.sleep(float(attempt) * self.BACKOFF) |
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.
The problem with this loop is that, in the worst case scenario, where every attempt failed, you end up sleeping before leaving the loop.
tests/test_client.py
Outdated
@@ -449,7 +449,7 @@ def test_call_rpc(self): | |||
self._mock_http(d, status=(504, "gateway timeout")) | |||
self.assertRaises(api.ProtocolError, self.sg._call_rpc, "list", a) | |||
self.assertEqual( | |||
4, | |||
3, |
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.
what's this change?
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.
I standardize the number of retries to be reused on the library.
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 use self.sgMAX_ATTEMPTS
instead or does it make sense to keep these 3 everywhere?
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.
I think it's feasible.
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.
LGTM!