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.

Sign up for GitHub

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

Jump to bottom

fix: retry non-idempotent long-running RPCs #141

Merged
merged 5 commits into from Apr 13, 2020
Merged

fix: retry non-idempotent long-running RPCs #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5bbb6fc
fix: retry non-idempotent long-running RPCs
olavloite Apr 7, 2020
e329a1b
fix: remove unnecessary changes
olavloite Apr 7, 2020
cd96869
tests: retry restore if it fails on max pending restores
olavloite Apr 7, 2020
1e9c4a6
fix: fix retry condition test
olavloite Apr 7, 2020
605b672
fix: use create time of database for retries
olavloite Apr 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
import com.google.api.gax.longrunning.OperationSnapshot;
import com.google.api.gax.longrunning.OperationTimedPollAlgorithm;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.StatusCode;
import com.google.api.gax.rpc.TransportChannelProvider;
import com.google.api.gax.rpc.UnaryCallSettings;
import com.google.cloud.NoCredentials;
Expand Down Expand Up @@ -291,7 +292,8 @@ private Builder() {
.setRetrySettings(longRunningRetrySettings);
databaseAdminStubSettingsBuilder
.updateBackupSettings()
.setRetrySettings(longRunningRetrySettings);
.setRetrySettings(longRunningRetrySettings)
.setRetryableCodes(StatusCode.Code.DEADLINE_EXCEEDED, StatusCode.Code.UNAVAILABLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to manually specify the retryable codes here? In the gapic config file (https://github.com/googleapis/googleapis/blob/d741cd976975c745d0199987aff0e908b8352992/google/spanner/admin/database/v1/spanner_admin_database_grpc_service_config.json#L58-L67), updateBackup will retry DEADLINE_EXCEEDED and UNAVAILABLE. Shouldn't this code be auto-generated?

I guess Java gapic code generator is using a different config file: https://github.com/googleapis/googleapis/blob/master/google/spanner/admin/database/v1/spanner_admin_database_gapic.yaml#L83-L86.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: why do we manually set retrying settings instead of using the default values from the gapic config file?

Copy link
Collaborator Author

@olavloite olavloite Apr 9, 2020 •
edited

Choose a reason for hiding this comment

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

I had a lot of problems / transient errors using the default values when working on the backups feature. So I set the values manually to try to find settings that did work, so that we can set them back to the default config.

So the Java client should definitely not continue to use custom retry settings. Once we have verified that these settings work well, we should set these as the default in the config file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And yes, the Java client (and I think also the other clients, but I'm not 100% sure) use the retry settings in https://github.com/googleapis/googleapis/blob/master/google/spanner/admin/database/v1/spanner_admin_database_gapic.yaml. So that means for example that the default config for UpdateBackup is non-idempotent.

}

Builder(SpannerOptions options) {
Expand Down Expand Up @@ -581,6 +583,7 @@ public Builder setEmulatorHost(String emulatorHost) {
return this;
}

@SuppressWarnings("rawtypes")
@Override
public SpannerOptions build() {
// Set the host of emulator has been set.
Expand Down