Skip to content
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

feat: add full RetrySettings sample code to Settings classes #3056

Merged
merged 10 commits into from
Sep 4, 2024

Conversation

alicejli
Copy link
Contributor

Fixes #2596 ☕️

Here's what the SpeechSettings c.g.c. snapshot looks like:
Screenshot from 2024-07-23 14-45-22

@product-auto-label product-auto-label bot added size: l Pull request size is large. samples Issues that are directly related to samples. labels Jul 23, 2024
@alicejli alicejli requested review from blakeli0 and lqiu96 July 23, 2024 18:47
@lqiu96
Copy link
Contributor

lqiu96 commented Jul 24, 2024

I think the changes look good. You probably need to update showcase as well since it's complaining.

Additionally, is there a way to tell what kind of method the RPC is (specifically if it's an LRO RPC or not)? I think it may be beneficial to give the LROs different sample values. But if it's too hard to do that, then we can probably ignore it since it's not important/ not that much benefit.

@alicejli
Copy link
Contributor Author

alicejli commented Jul 24, 2024

I think the changes look good. You probably need to update showcase as well since it's complaining.

ahh thank you - updated.

Additionally, is there a way to tell what kind of method the RPC is (specifically if it's an LRO RPC or not)? I think it may be beneficial to give the LROs different sample values. But if it's too hard to do that, then we can probably ignore it since it's not important/ not that much benefit.

I think I can do this - do you have a set of sample Retry values for LROs that I can try to pop in?

@lqiu96
Copy link
Contributor

lqiu96 commented Jul 24, 2024

Additionally, is there a way to tell what kind of method the RPC is (specifically if it's an LRO RPC or not)? I think it may be beneficial to give the LROs different sample values. But if it's too hard to do that, then we can probably ignore it since it's not important/ not that much benefit.

I think I can do this - do you have a set of sample Retry values for LROs that I can try to pop in?

Let's try with these default values:

// Default values for LongRunningConfig fields.
private static final long LRO_DEFAULT_INITIAL_POLL_DELAY_MILLIS = 5000;
private static final double LRO_DEFAULT_POLL_DELAY_MULTIPLIER = 1.5;
private static final long LRO_DEFAULT_MAX_POLL_DELAY_MILLIS = 45000;
private static final long LRO_DEFAULT_TOTAL_POLL_TIMEOUT_MILLIS = 300000; // 5 minutes.
private static final double LRO_DEFAULT_MAX_RPC_TIMEOUT = 1.0;

For LROs, we only need those settings: https://github.com/googleapis/google-cloud-java/tree/main?tab=readme-ov-file#configuring-lro-timeouts and the rest can be ignored

@@ -57,10 +59,21 @@ import javax.annotation.Generated;
* .echoWithVersionMethodSettings()
* .getRetrySettings()
* .toBuilder()
* .setTotalTimeout(Duration.ofSeconds(30))
* .setInitialRetryDelayDuration(Duration.ofSeconds(1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to see that we are using the new java.time methods! cc: @diegomarquezp

@alicejli
Copy link
Contributor Author

@lqiu96 I added logic to set different RetrySettings if the methods are LRO, but none of the goldens are using it, as there's currently logic in (AbstractServiceSettingsComposer)[https://github.com/googleapis/sdk-platform-java/blob/main/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java#L136] to pick the first unary method as the example method.

I did update the unit tests so you can see what the LRO example would look like, but I'm guessing it would be very unlikely that there is a service that only has LROs.

@lqiu96
Copy link
Contributor

lqiu96 commented Jul 25, 2024

@lqiu96 I added logic to set different RetrySettings if the methods are LRO, but none of the goldens are using it, as there's currently logic in (AbstractServiceSettingsComposer)[https://github.com/googleapis/sdk-platform-java/blob/main/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java#L136] to pick the first unary method as the example method.

I did update the unit tests so you can see what the LRO example would look like, but I'm guessing it would be very unlikely that there is a service that only has LROs.

What I'm thinking is if we can find an LRO example and add the LRO equivalent of

 * EchoStubSettings.Builder echoSettingsBuilder = EchoStubSettings.newBuilder();
 * echoSettingsBuilder
 *     .echoSettings()
 *     .setRetrySettings(
 *         echoSettingsBuilder
 *             .echoSettings()
 *             .getRetrySettings()
 *             .toBuilder()
 *             .setInitialRetryDelayDuration(Duration.ofSeconds(1))
 *             .setInitialRpcTimeoutDuration(Duration.ofSeconds(5))
 *             .setMaxAttempts(5)
 *             .setMaxRetryDelayDuration(Duration.ofSeconds(30))
 *             .setMaxRpcTimeoutDuration(Duration.ofSeconds(60))
 *             .setRetryDelayMultiplier(1.3)
 *             .setRpcTimeoutMultiplier(1.5)
 *             .setTotalTimeoutDuration(Duration.ofSeconds(300))
 *             .build());
 * EchoStubSettings echoSettings = echoSettingsBuilder.build();

as an additional sample. I don't know how much additional work it is, but in my mind it looked be fairly simple. Might need a bit of modifications to be something like:

 * EchoStubSettings.Builder echoSettingsBuilder = EchoStubSettings.newBuilder();
 * echoSettingsBuilder
 *     .echoSettings()
 *     .setPollingAlgorithm(...)

from https://github.com/googleapis/google-cloud-java/tree/main?tab=readme-ov-file#configuring-lro-timeouts

If too much work, let's just create an issue and I'm fine with the above.

Copy link

snippet-bot bot commented Jul 26, 2024

Here is the summary of possible violations 😱

There are 2 possible violations for not having product prefix.

The end of the violation section. All the stuff below is FYI purposes only.


Here is the summary of changes.

You are about to add 10 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Jul 26, 2024
Copy link

Quality Gate Failed Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
5.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@blakeli0 blakeli0 merged commit 8fe3a2d into main Sep 4, 2024
34 of 36 checks passed
@blakeli0 blakeli0 deleted the updateRetrySettingsSampleCode branch September 4, 2024 19:45
ldetmer added a commit that referenced this pull request Sep 9, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.45.0</summary>

##
[2.45.0](v2.44.0...v2.45.0)
(2024-09-09)


### Features

* add Batcher#close(timeout) and Batcher#cancelOutstanding
([#3141](#3141))
([b5a92e4](b5a92e4))
* add full RetrySettings sample code to Settings classes
([#3056](#3056))
([8fe3a2d](8fe3a2d))
* add toString to futures returned by operations
([#3140](#3140))
([afecb8c](afecb8c))
* bake gapic-generator-java into the hermetic build docker image
([#3067](#3067))
([a372e82](a372e82))


### Bug Fixes

* **gax:** prevent truncation/overflow when converting time values
([#3095](#3095))
([699074e](699074e))


### Dependencies

* add opentelemetry exporter-metrics and shared-resoucemapping to shared
dependencies
([#3078](#3078))
([fc8d80d](fc8d80d))
* update dependency certifi to v2024.8.30
([#3150](#3150))
([c18b705](c18b705))
* update dependency com.google.api-client:google-api-client-bom to
v2.7.0
([#3151](#3151))
([5f43e43](5f43e43))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3153](#3153))
([3071509](3071509))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3154](#3154))
([335ee63](335ee63))
* update dependency com.google.guava:guava to v33.3.0-jre
([#3119](#3119))
([41174b0](41174b0))
* update dependency dev.cel:cel to v0.7.1
([#3155](#3155))
([b1ddd16](b1ddd16))
* update dependency filelock to v3.16.0
([#3175](#3175))
([6681113](6681113))
* update dependency idna to v3.8
([#3156](#3156))
([82f5326](82f5326))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.66.final
([#3148](#3148))
([a7efaa8](a7efaa8))
* update dependency net.bytebuddy:byte-buddy to v1.15.1
([#3115](#3115))
([0e06c5f](0e06c5f))
* update dependency org.apache.commons:commons-lang3 to v3.17.0
([#3157](#3157))
([8d3b9fd](8d3b9fd))
* update dependency org.checkerframework:checker-qual to v3.47.0
([#3166](#3166))
([365674d](365674d))
* update dependency org.yaml:snakeyaml to v2.3
([#3158](#3158))
([e67ea9a](e67ea9a))
* update dependency platformdirs to v4.3.2
([#3176](#3176))
([4f2f9e0](4f2f9e0))
* update dependency virtualenv to v20.26.4
([#3177](#3177))
([080e078](080e078))
* update google api dependencies
([#3118](#3118))
([67342ea](67342ea))
* update google auth library dependencies to v1.25.0
([#3168](#3168))
([715884a](715884a))
* update google http client dependencies to v1.45.0
([#3159](#3159))
([a3fe612](a3fe612))
* update googleapis/java-cloud-bom digest to 6626f91
([#3147](#3147))
([658e40e](658e40e))
* update junit5 monorepo to v5.11.0
([#3111](#3111))
([6bf84c8](6bf84c8))
* update netty dependencies to v4.1.113.final
([#3165](#3165))
([9b5957d](9b5957d))
* update opentelemetry-java monorepo to v1.42.0
([#3172](#3172))
([413c44e](413c44e))


### Documentation

* Update DEVELOPMENT.md
([#3126](#3126))
([92bdf4e](92bdf4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: ldetmer <1771267+ldetmer@users.noreply.github.com>
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
ldetmer added a commit that referenced this pull request Sep 17, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.45.0</summary>

##
[2.45.0](v2.44.0...v2.45.0)
(2024-09-09)


### Features

* add Batcher#close(timeout) and Batcher#cancelOutstanding
([#3141](#3141))
([b5a92e4](b5a92e4))
* add full RetrySettings sample code to Settings classes
([#3056](#3056))
([8fe3a2d](8fe3a2d))
* add toString to futures returned by operations
([#3140](#3140))
([afecb8c](afecb8c))
* bake gapic-generator-java into the hermetic build docker image
([#3067](#3067))
([a372e82](a372e82))


### Bug Fixes

* **gax:** prevent truncation/overflow when converting time values
([#3095](#3095))
([699074e](699074e))


### Dependencies

* add opentelemetry exporter-metrics and shared-resoucemapping to shared
dependencies
([#3078](#3078))
([fc8d80d](fc8d80d))
* update dependency certifi to v2024.8.30
([#3150](#3150))
([c18b705](c18b705))
* update dependency com.google.api-client:google-api-client-bom to
v2.7.0
([#3151](#3151))
([5f43e43](5f43e43))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3153](#3153))
([3071509](3071509))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3154](#3154))
([335ee63](335ee63))
* update dependency com.google.guava:guava to v33.3.0-jre
([#3119](#3119))
([41174b0](41174b0))
* update dependency dev.cel:cel to v0.7.1
([#3155](#3155))
([b1ddd16](b1ddd16))
* update dependency filelock to v3.16.0
([#3175](#3175))
([6681113](6681113))
* update dependency idna to v3.8
([#3156](#3156))
([82f5326](82f5326))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.66.final
([#3148](#3148))
([a7efaa8](a7efaa8))
* update dependency net.bytebuddy:byte-buddy to v1.15.1
([#3115](#3115))
([0e06c5f](0e06c5f))
* update dependency org.apache.commons:commons-lang3 to v3.17.0
([#3157](#3157))
([8d3b9fd](8d3b9fd))
* update dependency org.checkerframework:checker-qual to v3.47.0
([#3166](#3166))
([365674d](365674d))
* update dependency org.yaml:snakeyaml to v2.3
([#3158](#3158))
([e67ea9a](e67ea9a))
* update dependency platformdirs to v4.3.2
([#3176](#3176))
([4f2f9e0](4f2f9e0))
* update dependency virtualenv to v20.26.4
([#3177](#3177))
([080e078](080e078))
* update google api dependencies
([#3118](#3118))
([67342ea](67342ea))
* update google auth library dependencies to v1.25.0
([#3168](#3168))
([715884a](715884a))
* update google http client dependencies to v1.45.0
([#3159](#3159))
([a3fe612](a3fe612))
* update googleapis/java-cloud-bom digest to 6626f91
([#3147](#3147))
([658e40e](658e40e))
* update junit5 monorepo to v5.11.0
([#3111](#3111))
([6bf84c8](6bf84c8))
* update netty dependencies to v4.1.113.final
([#3165](#3165))
([9b5957d](9b5957d))
* update opentelemetry-java monorepo to v1.42.0
([#3172](#3172))
([413c44e](413c44e))


### Documentation

* Update DEVELOPMENT.md
([#3126](#3126))
([92bdf4e](92bdf4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: ldetmer <1771267+ldetmer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
samples Issues that are directly related to samples. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate snippets for RetrySettings
3 participants