Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: deprecate RetrySettings.isJittered [gax-java] #1308

Merged
merged 3 commits into from Feb 19, 2021
Merged

feat: deprecate RetrySettings.isJittered [gax-java] #1308

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af75910
feat!: deprecate RetrySettings.isJittered
miraleung Feb 18, 2021
5333d89
fix: Address comments
miraleung Feb 19, 2021
b454dce
Merge branch 'master' of github.com:googleapis/gax-java into feat/dep…
miraleung Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java
View file
Open in desktop
Expand Up @@ -187,7 +187,7 @@ public boolean shouldRetry(TimedAttemptSettings nextAttemptSettings) {

// Injecting Random is not possible here, as Random does not provide nextLong(long bound) method
protected long nextRandomLong(long bound) {
return bound > 0 && globalSettings.isJittered()
return bound > 0 && globalSettings.isJittered() // Jitter check needed for testing purposes.
? ThreadLocalRandom.current().nextLong(bound)
: bound;
}
Expand Down
9 changes: 9 additions & 0 deletions gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@
package com.google.api.gax.retrying;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import java.io.Serializable;
import org.threeten.bp.Duration;

Expand Down Expand Up @@ -112,7 +113,11 @@ public abstract class RetrySettings implements Serializable {
* <pre>{@code actualDelay = rand_between(0, min(maxRetryDelay, delay))}</pre>
*
* The default value is {@code true}.
*
* @deprecated Retries will always jitter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will always --> always
per Google tech writing guidelines

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

*/
@Deprecated
@VisibleForTesting
public abstract boolean isJittered();

/**
Expand Down Expand Up @@ -200,7 +205,11 @@ public abstract static class Builder {
* <pre>{@code actualDelay = rand_between(0, min(maxRetryDelay, exponentialDelay))}</pre>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment should be updated to specify it's a no-op

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

*
* The default value is {@code true}.
*
* @deprecated Retries will always jitter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true already? I would have expected some updates to concrete methods are also necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
@Deprecated
@VisibleForTesting
public abstract Builder setJittered(boolean jittered);

/**
Expand Down