gapic-generator-java icon indicating copy to clipboard operation
gapic-generator-java copied to clipboard
verified publisher icon googleapis

feat: add full RetrySettings sample code to Settings classes

Open alicejli opened this issue 1 year ago • 8 comments

Fixes #2596 ☕️

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

alicejli avatar Jul 23 '24 18:07 alicejli

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.

lqiu96 avatar Jul 24 '24 14:07 lqiu96

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?

alicejli avatar Jul 24 '24 15:07 alicejli

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: https://github.com/googleapis/sdk-platform-java/blob/43de5b568dc0bbdaddf2419652e0cb16de77ddb6/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/RetrySettingsComposer.java#L68-L73

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

lqiu96 avatar Jul 24 '24 16:07 lqiu96

@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.

alicejli avatar Jul 25 '24 19:07 alicejli

@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.

lqiu96 avatar Jul 25 '24 21:07 lqiu96

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

snippet-bot[bot] avatar Jul 26 '24 21:07 snippet-bot[bot]

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

sonarqubecloud[bot] avatar Jul 26 '24 21:07 sonarqubecloud[bot]

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

sonarqubecloud[bot] avatar Jul 26 '24 21:07 sonarqubecloud[bot]