-
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: check if retry is allowed after retry wait calculation #258
Conversation
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.
This looks good but I'm concerned at why there aren't more tests we have to change to reflect this. Do we not have any tests that test the case where retry strategy max_retries is used, instead of the timer-based settings? Do we need to add one, as a regression test?
|
||
# Check if (another) retry is allowed. If retries are exhausted and | ||
# no acceptable response was received, raise the retriable error. | ||
if not retry_strategy.retry_allowed(total_sleep, num_retries): |
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 docstring for retry_strategy.retry_allowed()
says that the num_retries is supposed to be the number of retries used so far. After thinking about it, I agree with your changes here and think that docstring is wrong. We should modify the docstring to reflect 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.
@andrewsg I've updated the docstring and added another test case. PTAL, thanks!
…le-media-python into count-retry-allow
_helpers.wait_and_retry()
handles many of the retry calls in the library and utilizesretry_allowed()
. Given the logic inretry_allowed()
, the check should happen after the num_retries and total_sleep incrementation.For example, if
RetryStrategy.max_retries = 0
, the check should prevent any additional retry attempts. With the previous logic, 1 retry call would be made.If
RetryStrategy.max_cumulative_retry = 100
, the check should prevent another retry that will cause the total sleep time to exceed 100. Moving the order of the retry_allowed check resolves this.Fixes #256