docs: Update RetrySettings javadocs to include polling #1863
Conversation
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!
|
| * Duration.ZERO}. | ||
| * | ||
| * <p>If there are no configurations, Retries have the default initial retry delay value set above | ||
| * and LROs have a default initial poll delay value of {@code Duration.ofMillis(5000)} (5 |
There was a problem hiding this comment.
Where is the default number coming from? Can we link to where the default number is actually specified? Same comment for all the default numbers below.
There was a problem hiding this comment.
I have a reference here:
, but it's at the top of the file and may get lost. The default value retry values are populated from the grpc config file and I don't want to link those in here. LROs are preconfigured from the generation.There was a problem hiding this comment.
What are the default numbers if there is no gRPC config file? I guess the numbers are coming from constants in GAX ? The concern I have is if we change the default numbers in the future, we would have to change the numbers here as well, and it's very easy to forget to change here.
There was a problem hiding this comment.
If there is nothing configured inside the grpc config file, the generator will create a RetrySettings with the default values with values listed above (For example: https://github.com/googleapis/google-cloud-java/blob/8452ac3a8eefa8a9e49ff83187b33be26efe6a01/java-batch/google-cloud-batch/src/main/java/com/google/cloud/batch/v1/stub/BatchServiceStubSettings.java#L518). This is what I'll call the RetrySettings default values.
Service teams can modify the grpc config file and that will provide default values for certain callables. These default values is what I'll call the callable default values. This is the RetrySettings which will be configured like: https://github.com/googleapis/google-cloud-java/blob/8452ac3a8eefa8a9e49ff83187b33be26efe6a01/java-batch/google-cloud-batch/src/main/java/com/google/cloud/batch/v1/stub/BatchServiceStubSettings.java#L507-L516
I believe they're both technically default values. The Javadoc I updated only refers the RetrySettings default values since the callable default values differ per service. I can add this explanation in so it's clearer to the readers.
There was a problem hiding this comment.
The default numbers in RetrySettingsComposer is what I was referring to. If those numbers change, we would have to update the numbers here as well. But since those numbers are in the generator, I don't think it's a good idea to expose the implementation details to Gax either, I think we can leave them as they are for now.
There was a problem hiding this comment.
The default numbers in RetrySettingsComposer is what I was referring to. If those numbers change, we would have to update the numbers here as well. But since those numbers are in the generator, I don't think it's a good idea to expose the implementation details to Gax either, I think we can leave them as they are for now.
Yeah, those values are set in the generator (and the generator only sets defaults for LROs). I think if we make an effort to change those, we'll have to remember to update our docs here manually. I'd like for an automated way to do that, but linking to the generator code doesn't seem ideal.
I'll keep as is for now then.
gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java
Outdated
Show resolved
Hide resolved
| * | ||
| * <p>If there are no configurations, Retries have the default max retry delay value set above | ||
| * and LROs have a default max poll retry delay value of {@code Duration.ofMillis(45000)} (45 | ||
| * seconds). | ||
| */ | ||
| public abstract Builder setMaxRetryDelay(Duration maxDelay); | ||
|
|
||
| /** | ||
| * MaxAttempts defines the maximum number of attempts to perform. The default value is {@code | ||
| * 0}. If this value is greater than 0, and the number of attempts reaches this limit, the logic |
There was a problem hiding this comment.
I remember that both 0 and 1 mean there is no retry from a YAQS. It's kind of already implied as maxAttempts should include the first attempt, can you please make it clear in the comment here to reduce similar confusion in the future?
There was a problem hiding this comment.
Updated. Let me know if this makes more sense.
There was a problem hiding this comment.
The first attempt will be considered attempt #0. I think this is true if maxAttempts is set to 0, but The first attempt will be considered attempt #1 if maxAttempts is set to 1?
There was a problem hiding this comment.
The first attempt will be set as Attempt #0 (
nextAttempt class and update the attemptCount: I think it would make sense if I specify that maxAttempts refers to the number of retry attempts instead.
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!
|
Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #2214