Merged

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

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Failed to load files.
Original file line numberDiff line numberDiff line change
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 DownExpand 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://.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://.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
Loading

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://.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 DownExpand 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
Loading