fix: allows retries when setting maxAttempts without setting TotalTimeout #835
Conversation
Codecov Report
@@ Coverage Diff @@
## master #835 +/- ##
============================================
- Coverage 78.72% 78.61% -0.11%
+ Complexity 1146 1143 -3
============================================
Files 202 202
Lines 5099 4517 -582
Branches 405 374 -31
============================================
- Hits 4014 3551 -463
+ Misses 912 794 -118
+ Partials 173 172 -1
Continue to review full report at Codecov.
|
gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java
Outdated
Show resolved
Hide resolved
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!
@vam-google, can you take a look as well?
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.
Looking at the original issue it is not clear why the decision was made to modify the algorithm itself but not how the retry settings builder behaves.
Also, this looks like backward incompatible behavior change for a GA component.
gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java
Show resolved
Hide resolved
gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #835 +/- ##
============================================
+ Coverage 78.72% 78.73% +0.01%
- Complexity 1146 1151 +5
============================================
Files 202 202
Lines 5099 5107 +8
Branches 405 407 +2
============================================
+ Hits 4014 4021 +7
- Misses 912 913 +1
Partials 173 173
Continue to review full report at Codecov.
|
I think the underlying issue is that the algorithm assumes that there is always a total timeout, which is why it needs to be updated |
…ry would not attempt any retries if totalTimeout & maxAttempt is 0.
@igorbernstein2 @rahulKQL I think if the case when both |
@rahulKQL can you update the RetrySettings docs? I think updating this this paragraph from
To:
|
@rahulKQL can you also update the definition of gax-java/gax/src/main/java/com/google/api/gax/rpc/Callables.java Lines 249 to 251 in 8d7aafc
|
@igorbernstein2 @vam-google Thanks for the review, I have updated this change request accordingly. Could you please have a fresh look! |
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
Fixes #827