-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix: Only override built-in retry settings when the customer provides them. #1588
Conversation
🤖 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a unit test for this?
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I will copy/paste that.
Internal tracking: b/283455016
The retry settings found in
FirestoreStubSettings
are 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
maxAttempts
is 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.