Spanner: gRPC Retry and Timeout Settings should be used from configuration

[crwilcox]

https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/v1/stub/SpannerStubSettings.java#L445

provides retry settings for methods. These should be relied on as they are kept in sync between the different client libraries.

Currently these constants are being used and should likely instead reference the configs https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java#L62

[olavloite]

The Java client library for Spanner has been changed to use the gapic client and rely on Gax for almost all retries, so these constants are not used very much anymore. There are two exceptions:

• Retrying aborted transactions: This is currently using the SpannerRetryHelper.
• Retrying broken streams for result sets: This is currently using the constant referenced in this issue here: https://github.com/googleapis/google-cloud-java/blob/03f4bec198366b84516d991d99df85715293d7c5/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java#L918

The retry settings from gapic are defined for the different gRPC methods. The two exceptions above are not directly related to one method, but could be caused by different methods, so there is no direct choice for which retry setting from the config that should be used. My suggestion would be:

• Retrying transactions: Use the 'default' settings https://github.com/googleapis/google-cloud-java/blob/03f4bec198366b84516d991d99df85715293d7c5/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/v1/stub/SpannerStubSettings.java#L394
• Retrying broken streams: Use the 'streaming' settings https://github.com/googleapis/google-cloud-java/blob/03f4bec198366b84516d991d99df85715293d7c5/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/v1/stub/SpannerStubSettings.java#L405

@kolea2 and @crwilcox Do you both agree with that?

[skuruppu]

The Java client library for Spanner has been changed to use the gapic client and rely on Gax for almost all retries, so these constants are not used very much anymore. There are two exceptions:

• Retrying aborted transactions: This is currently using the SpannerRetryHelper.
• Retrying broken streams for result sets: This is currently using the constant referenced in this issue here: https://github.com/googleapis/google-cloud-java/blob/03f4bec198366b84516d991d99df85715293d7c5/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java#L918

The retry settings from gapic are defined for the different gRPC methods. The two exceptions above are not directly related to one method, but could be caused by different methods, so there is no direct choice for which retry setting from the config that should be used. My suggestion would be:

• Retrying transactions: Use the 'default' settings https://github.com/googleapis/google-cloud-java/blob/03f4bec198366b84516d991d99df85715293d7c5/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/v1/stub/SpannerStubSettings.java#L394
• Retrying broken streams: Use the 'streaming' settings https://github.com/googleapis/google-cloud-java/blob/03f4bec198366b84516d991d99df85715293d7c5/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/v1/stub/SpannerStubSettings.java#L405

@kolea2 and @crwilcox Do you both agree with that?

I think this is reasonable @olavloite. Please go ahead with making the necessary changes.

[olavloite]

All custom retry values have now been replaced by values from the gapic configuration by #48 and #49.