-
Notifications
You must be signed in to change notification settings - Fork 33
Add retry support #1925
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
Add retry support #1925
Conversation
…lient-java into hantingz/add-retry
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! 2 other changes I'd like to see:
- Be able to mock sleep in the test. Maybe add a sleep method on RetryConfig, so that you can override it in tests
- Add a test for 500
src/test/resources/cassettes/v2/RetryTest.retryWithDashboardListItemGetTest429.json
Show resolved
Hide resolved
| if (statusCode == 429 || statusCode >= 500) { | ||
| statusToRetry = true; | ||
| } | ||
| return (retryConfig.maxRetries >= retryCount && statusToRetry && retryConfig.isEnableRetry()); |
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.
| return (retryConfig.maxRetries >= retryCount && statusToRetry && retryConfig.isEnableRetry()); | |
| return (retryConfig.maxRetries > retryCount && statusToRetry && retryConfig.isEnableRetry()); |
As you start with 0 this should be a strict comparison.
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 still true?
README.md
Outdated
| To enable the client to retry when rate limited (status 429) or status 500 and above: | ||
|
|
||
| ```java | ||
| defaultClient.setRetry(new RetryConfig(true, int <multiplier_for_retry_backoff>, int <base_for_retry_backoff>,int <max_retry_attemps>)) |
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.
Let's document enableRetry here insread.
| if (statusCode == 429 || statusCode >= 500) { | ||
| statusToRetry = true; | ||
| } | ||
| return (retryConfig.maxRetries >= retryCount && statusToRetry && retryConfig.isEnableRetry()); |
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 still true?
What does this PR do?
Adds retry support and corresponding config options.
Additional Notes
Review checklist
Please check relevant items below:
This PR includes all newly recorded cassettes for any modified tests.
This PR does not rely on API client schema changes.
Or, this PR relies on API schema changes and this is a Draft PR that includes tests.