[WIP] Spanner - Use gax for runWithRetries()

[olavloite]

Instead of using its own retry logic, SpannerImpl should use the retry helper and options from gax.
This PR is based on #4749 Spanner refactor spannerimpl, which should be processed first.

Issues / discussion:

SpannerImpl already uses io.grpc.Context to enable setting timeouts and cancelling database operations. This will interfere with using the functionality from gax. The implementation in this PR supports both, but the behavior is different from the previous version if the user for example sets a short timeout on Context in combination with a longer timeout on RetrySettings. If the timeout of RetrySettings is longer than the timeout set on Context, the context timeout will only occur after the timeout of RetrySettings has occurred (i.e. the time before the timeout exception is thrown is longer than the user would have expected).

Possible solutions as I see them at this moment:

1. Remove support for using io.grpc.Context for setting timeouts and cancelling database operations, and only rely on the functionality from gax (which use the RetrySettings). This would change the behavior for clients who currently use this to set a timeout or to cancel calls.
2. Use the deadline from io.grpc.Context if one is set (or if the current context is a CancellableContext), and use gax in all other cases. This basically means 'use the current functionality if the user has set a deadline on the Context or indicated that it should be cancellable, and use RetrySettings in all other cases'.
3. Do not migrate this functionality to use gax.

[codecov]

Codecov Report

Merging #4781 into master will increase coverage by 0.63%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #4781 +/- ##
============================================
+ Coverage 49.44% 50.07% +0.63% - Complexity 21501 22813 +1312 
============================================
Files 2168 2168 Lines 212334 216773 +4439 Branches 23409 24263 +854 ============================================
+ Hits 104987 108548 +3561 - Misses 99134 99882 +748 - Partials 8213 8343 +130

Impacted Files	Coverage Δ	Complexity Δ
...re/src/main/java/com/google/cloud/BaseService.java	55.55% <0%> (-6.95%)	2% <0%> (ø)
...re/src/main/java/com/google/cloud/RetryHelper.java	50% <0%> (-3.85%)	2% <0%> (ø)
...le/cloud/compute/deprecated/DeprecationStatus.java	88.63% <0%> (-3.13%)	20% <0%> (ø)
...m/google/cloud/tasks/v2beta3/CloudTasksClient.java	57.43% <0%> (-2.68%)	67% <0%> (+16%)
...m/google/cloud/tasks/v2beta2/CloudTasksClient.java	57.14% <0%> (-2.39%)	79% <0%> (+19%)
.../cloud/scheduler/v1beta1/CloudSchedulerClient.java	55% <0%> (-2.15%)	35% <0%> (+8%)
...oogle/cloud/scheduler/v1/CloudSchedulerClient.java	55% <0%> (-2.15%)	35% <0%> (+8%)
...oogle/cloud/firestore/v1/FirestoreAdminClient.java	64.6% <0%> (-2.07%)	36% <0%> (+6%)
.../com/google/cloud/automl/v1beta1/AutoMlClient.java	59.7% <0%> (-2.04%)	59% <0%> (+14%)
...gle/cloud/bigtable/data/v2/BigtableDataClient.java	89.58% <0%> (-1.91%)	32% <0%> (ø)
... and 521 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e706ba1...c9d5aeb. Read the comment docs.

[kolea2]

are these chances part of the overall refactoring?

[olavloite]

Yes, this class used to be an inner class of SpannerImpl. I based this change on the branch of the refactor PR, as they would otherwise cause a lot of merge conflicts between the two of them.

If you only want to see the changes made after the overall refactoring, you can use this link: olavloite/google-cloud-java@spanner-refactor-spannerimpl...olavloite:spanner-gax-retry

The most important change is in SpannerImpl.java. The other changes are changes that are necessary as the new retry method is not static, as gax uses the ServiceOptions that have been used to create the Spanner instance.

[ajaaym]

@olavloite ApiFuture returned by callable do support cancellation of calls. Also with every call to callable, we can provide call context with deadline. I think whole of retrying logic can be migrated to gax and configured in handwritten client.

[olavloite]

@olavloite ApiFuture returned by callable do support cancellation of calls. Also with every call to callable, we can provide call context with deadline. I think whole of retrying logic can be migrated to gax and configured in handwritten client.

@ajaaym Yes, I agree with you on that, but as far as I can see we will be changing the behavior for users that currently set a deadline or cancellation themselves. Consider the code in SpannerImplRetryTest.contextDeadlineExceeded() and assume that this was not a test case, but code written by a user. Moving the retry logic of Spanner from the current implementation to gax will lead to the question: Which deadline should we apply to a database operation. The one specified in the RetrySettings of the SpannerOptions that was used to create the Spanner instance, or the one the user has set on the call context? Gax will by default use the one specified in RetrySettings.

Or am I missing something here?

[olavloite]

Replaced by #4940