fix: Only override built-in retry settings when the customer provides them.#1588
fix: Only override built-in retry settings when the customer provides them.#1588tom-andersen merged 7 commits intomainfrom
Conversation
product-auto-label
bot
added
size: s
tom-andersen
changed the title
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
tom-andersen
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
ehsannas
left a comment
ehsannas
left a comment
There was a problem hiding this comment.
Is it possible to add a unit test for this?
product-auto-label
bot
added
size: l
tom-andersen
added
the
kokoro:force-run
@ehsannas I added test in order to verify that retry settings are being passed correctly to the calling layer. It relies heavily on reflection, so there is a good chance this will break and need to be fixed with new versions of GAX. But hopefully, that will be manageable. |
yoshi-kokoro
removed
the
kokoro:force-run
ehsannas
left a comment
ehsannas
left a comment
There was a problem hiding this comment.
LGTM. Some nits pointed out below.
| .setInitialRetryDelay(Duration.ofMillis(100)) | ||
| .setMaxRetryDelay(Duration.ofSeconds(60)) | ||
| .setRetryDelayMultiplier(1.3) | ||
| .build(); |
There was a problem hiding this comment.
Aren't expectedRetrySettings1 and expectedRetrySettings4 and expectedRetrySettings5 the same? should we use the same variable for them?
| } | ||
|
|
||
| @Test | ||
| public void batchWriteCallableFollowsServiceConfigFollowsServiceConfig() throws Exception { |
There was a problem hiding this comment.
FollowsServiceConfigFollowsServiceConfig
| // Override retry settings only if customer provides settings different from default. | ||
| if (retrySettings.equals(ServiceOptions.getDefaultRetrySettings())) { | ||
| // This code should be removed when following issue is fixed: | ||
| // https://github.com/googleapis/sdk-platform-java/issues/2306 |
There was a problem hiding this comment.
Someone reading the code for the first time may wonder where these setMaxAttempts(5) are coming from.
Perhaps the following is a better comment:
// We are manually setting `setMaxAttempts(5)` to follow
// the `firestore_grpc_service_config.json` configuration.
// This code should be removed when following issue is fixed:
// https://github.com/googleapis/sdk-platform-java/issues/2306
There was a problem hiding this comment.
Nice. I will copy/paste that.
tom-andersen
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
3 participants
Internal tracking: b/283455016
The retry settings found in
FirestoreStubSettingsare being overridden, whether or not the customer provides an override. To prevent this, we conditionally override based on whether customer has provided retry settings.In addition, default for
maxAttemptsis omitted from automatically generated in code. There is a link to issue in code. Until this issue is resolved, we will need to overridemaxAttempts, but everything else should be according to build-in config.Also, the new streaming endpoint AggregateQuery never had the retry config applied. That is also fixed in this PR.