Merged
Show file tree
Hide file tree
Changes from all commits
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

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