feat: deprecate RetrySettings.isJittered [gax-java] #1308
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1308 +/- ##
============================================
- Coverage 79.51% 79.39% -0.13%
+ Complexity 1238 1233 -5
============================================
Files 209 209
Lines 5434 5397 -37
Branches 454 438 -16
============================================
- Hits 4321 4285 -36
Misses 933 933
+ Partials 180 179 -1
Continue to review full report at Codecov.
|
| @@ -112,7 +113,11 @@ | |||
| * <pre>{@code actualDelay = rand_between(0, min(maxRetryDelay, delay))}</pre> | |||
| * | |||
| * The default value is {@code true}. | |||
| * | |||
| * @deprecated Retries will always jitter. | |||
There was a problem hiding this comment.
will always --> always
per Google tech writing guidelines
| @@ -200,7 +205,11 @@ public static Builder newBuilder() { | |||
| * <pre>{@code actualDelay = rand_between(0, min(maxRetryDelay, exponentialDelay))}</pre> | |||
| * | |||
| * The default value is {@code true}. | |||
| * | |||
| * @deprecated Retries will always jitter. | |||
There was a problem hiding this comment.
Is this true already? I would have expected some updates to concrete methods are also necessary.
There was a problem hiding this comment.
If we eventually remove the public scoping, then yes. Only ExponentialRetryAlgorithm does anything with this at the moment, but that cannot be changed without removing the test cases mentioned in the PR description.
| @@ -200,7 +205,11 @@ public static Builder newBuilder() { | |||
| * <pre>{@code actualDelay = rand_between(0, min(maxRetryDelay, exponentialDelay))}</pre> | |||
There was a problem hiding this comment.
comment should be updated to specify it's a no-op
There was a problem hiding this comment.
It looks like testTruncateToTotalTimeout should pass even without setting the jitter since it does a relative comparison. testCreateNextAttempt is too strict a test. We should not test exact values of timeouts.
We can fix that later though.
tests are failing, and this needs merge from master.
…recate_isjittered
Agree that those can be fixed, but OperationCallableImplTest is the real bottleneck - the operations never complete without turning off jitter. |
Could you please elaborate on what else we need (aside from updating the two tests)? |
|
The request in #1292 is to always jitter unconditionally. The client should not be able to disable this. |
|
This was marked as a breaking change - is this intentional? It's going to be releasing a 2.0.0 |
elharo
changed the title
|
No, it shouldn't be. It deprecates but does not remove or change anything. |
This should fix the title of the release-please PR. #1308 should not have been marked as breaking Release-As: 1.62.0
🤖 I have created a release \*beep\* \*boop\* --- ## [1.62.0](https://www.github.com/googleapis/gax-java/compare/v1.61.0...v1.62.0) (2021-02-25) ### Features * deprecate RetrySettings.isJittered [gax-java] ([#1308](https://www.github.com/googleapis/gax-java/issues/1308)) ([68644a4](https://www.github.com/googleapis/gax-java/commit/68644a4e24f29223f8f533a3d353dff7457d9737)) * dynamic flow control part 1 - add FlowController to Batcher ([#1289](https://www.github.com/googleapis/gax-java/issues/1289)) ([bae5eb6](https://www.github.com/googleapis/gax-java/commit/bae5eb6070e690c26b95e7b908d15300aa54ef1c)) ### Bug Fixes * prevent unchecked warnings in gax-httpjson ([#1306](https://www.github.com/googleapis/gax-java/issues/1306)) ([ee370f6](https://www.github.com/googleapis/gax-java/commit/ee370f62c5d411738a9b25cf4cfc095aa06d9e07)) * remove unused @InternalExtensionOnly from CallContext classes ([#1304](https://www.github.com/googleapis/gax-java/issues/1304)) ([a8d3a2d](https://www.github.com/googleapis/gax-java/commit/a8d3a2dca96efdb1ce154a976c3e0844e3f501d6)) ### Dependencies * update google-auth-library to 0.24.0 ([#1315](https://www.github.com/googleapis/gax-java/issues/1315)) ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72)) * update google-common-protos to 2.0.1 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72)) * update google-http-client to 1.39.0 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72)) * update google-iam ([#1313](https://www.github.com/googleapis/gax-java/issues/1313)) ([327b53c](https://www.github.com/googleapis/gax-java/commit/327b53ca7739d9be6e24305b23af2c7a35cb6f4d)) * update gRPC to 1.36.0 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72)) * update opencensus to 0.28.0 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72)) * update protobuf to 3.15.2 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
starts work on #1292
In the future, full deprecation will involve removing the public scoping, since this is still needed to (a) terminate polling for OperationCallableImplTest, and (b) set deterministic values for ExpontentialRetryAlgorithmTest's testCreateNextAttempt and testTruncateToTotalTimeout.