-
Notifications
You must be signed in to change notification settings - Fork 25
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 policy config #360
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #360 +/- ##
============================================
+ Coverage 57.72% 58.40% +0.67%
- Complexity 2055 2111 +56
============================================
Files 311 313 +2
Lines 12550 12628 +78
Branches 1245 1252 +7
============================================
+ Hits 7245 7375 +130
+ Misses 4712 4658 -54
- Partials 593 595 +2 ☔ View full report in Codecov by Sentry. |
common/src/main/java/tech/ydb/common/retry/YdbRetryBuilder.java
Outdated
Show resolved
Hide resolved
Fixes #81 |
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
It would be nice have a test case for non-retryable error appearing after write()
call completion to check what would happen with the returned CompletableFuture
.
Also, there is one small thing that could be polished.
Currently, there are two ways of disabling retries: one is by returning null from RetryConfig.getStatusRetryPolicy
and the other is returning a negative number from RetryPolicy.nextRetryMs
. Maybe we should keep only one way (right now I'm in favor of the first approach).
It was a conscious decision. Two different ways to reject the next retry describe two different situations: first if the unretryable error appears, and second if retry is rejected by policy, such as exceeding the maximum count of retries or the operation timeout. It helps us to debug different problems |
Fixes #81