Skip to content

[WIP] Spanner - Use gax for runWithRetries() #4781

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
olavloite wants to merge 4 commits into from
Closed

[WIP] Spanner - Use gax for runWithRetries() #4781

olavloite wants to merge 4 commits into from

Conversation

@olavloite
Copy link

@olavloite olavloite commented Mar 28, 2019

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.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 28, 2019
@codecov
Copy link

codecov bot commented Mar 28, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@             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
kolea2 reviewed Mar 28, 2019
View reviewed changes
...nts/google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceAdminClientImpl.java
@@ -0,0 +1,213 @@
/*
Copy link
Contributor

@kolea2 kolea2 Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these chances part of the overall refactoring?

Copy link
Author

@olavloite olavloite Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

ajaaym commented Mar 29, 2019

@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
Copy link
Author

olavloite commented Mar 29, 2019

@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?

@sduskis sduskis added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 29, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 4, 2019
@sduskis sduskis added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Apr 11, 2019
@olavloite
Copy link
Author

olavloite commented Apr 13, 2019

Replaced by #4940

@olavloite olavloite closed this Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kolea2 kolea2 kolea2 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing. needs work This is a pull request that needs a little love.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@olavloite @ajaaym @kolea2 @sduskis @googlebot @yoshi-automation