fix: retry non-idempotent long-running RPCs

[olavloite]

RPCs returning a long-running operation, such as CreateDatabase, CreateBackup and RestoreDatabase, are non-idempotent and cannot be retried automatically by gax. This means that these RPCs sometimes fail with transient errors, such as UNAVAILABLE or DEADLINE_EXCEEDED. This change introduces automatic retries of these RPCs using the following logic:

1. Execute the RPC and wait for the operation to be returned.
2. If a transient error occurs while waiting for the operation, the client library queries the backend for the corresponding operation. If the operation is found, the resumes the tracking of the existing operation and returns that to the user.
3. If no corresponding operation is found in step 2, the client library retries the RPC from step 1.

Fixes GoogleCloudPlatform/java-docs-samples#2571

[hengfengli]

LGTM.

[skuruppu]

I think we don't specify retry settings for non-idempotent RPCs. Do you know what this will default to then?

[olavloite]

There are defaults defined in the gapic configuration file as well, but those are already overridden by the client library. The defaults are set in the constructor of SpannerOptions.Builder:

java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java

Lines 242 to 295 in 9e5a1cd

	private Builder() {
	// Manually set retry and polling settings that work.
	OperationTimedPollAlgorithm longRunningPollingAlgorithm =
	OperationTimedPollAlgorithm.create(
	RetrySettings.newBuilder()
	.setInitialRpcTimeout(Duration.ofSeconds(60L))
	.setMaxRpcTimeout(Duration.ofSeconds(600L))
	.setInitialRetryDelay(Duration.ofSeconds(20L))
	.setMaxRetryDelay(Duration.ofSeconds(45L))
	.setRetryDelayMultiplier(1.5)
	.setRpcTimeoutMultiplier(1.5)
	.setTotalTimeout(Duration.ofHours(48L))
	.build());
	RetrySettings longRunningRetrySettings =
	RetrySettings.newBuilder()
	.setInitialRpcTimeout(Duration.ofSeconds(60L))
	.setMaxRpcTimeout(Duration.ofSeconds(600L))
	.setInitialRetryDelay(Duration.ofSeconds(20L))
	.setMaxRetryDelay(Duration.ofSeconds(45L))
	.setRetryDelayMultiplier(1.5)
	.setRpcTimeoutMultiplier(1.5)
	.setTotalTimeout(Duration.ofHours(48L))
	.build();
	databaseAdminStubSettingsBuilder
	.createDatabaseOperationSettings()
	.setPollingAlgorithm(longRunningPollingAlgorithm)
	.setInitialCallSettings(
	UnaryCallSettings
	.<CreateDatabaseRequest, OperationSnapshot>newUnaryCallSettingsBuilder()
	.setRetrySettings(longRunningRetrySettings)
	.build());
	databaseAdminStubSettingsBuilder
	.createBackupOperationSettings()
	.setPollingAlgorithm(longRunningPollingAlgorithm)
	.setInitialCallSettings(
	UnaryCallSettings
	.<CreateBackupRequest, OperationSnapshot>newUnaryCallSettingsBuilder()
	.setRetrySettings(longRunningRetrySettings)
	.build());
	databaseAdminStubSettingsBuilder
	.restoreDatabaseOperationSettings()
	.setPollingAlgorithm(longRunningPollingAlgorithm)
	.setInitialCallSettings(
	UnaryCallSettings
	.<RestoreDatabaseRequest, OperationSnapshot>newUnaryCallSettingsBuilder()
	.setRetrySettings(longRunningRetrySettings)
	.build());
	databaseAdminStubSettingsBuilder
	.deleteBackupSettings()
	.setRetrySettings(longRunningRetrySettings);
	databaseAdminStubSettingsBuilder
	.updateBackupSettings()
	.setRetrySettings(longRunningRetrySettings);
	}

[hengfengli]

So does this mean that the Java client library has its own retrying settings which are different from the gapic config file?

[hengfengli]

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.

[hengfengli]

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

[olavloite]

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.

[olavloite]

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.