Skip to content

Conversation

@olavloite
Copy link

@olavloite olavloite commented Apr 13, 2019

Use GAX instead of the custom SpannerImpl.runWithRetries method for retrying server calls.

This migration involves three changes:

  • Remove the override of the generated default settings in GapicSpannerRpc that set the retryable error codes to an empty set. This will automatically enable GAX retries for all server calls using the default values that have already been defined. The defaults are the same as the defaults that are used by SpannerClient.
  • Add configuration options to SpannerOptions to allow the user to change the default RetrySettings. As SpannerOptions is used to create a Spanner instance, which again contains both a DatabaseClient, DatabaseAdminClient and InstanceAdminClient, SpannerOptions must contain configuration options for all three underlying clients.
  • The runWithRetries method in SpannerImpl is removed and all references to it changed into direct calls that will automatically be retried by GAX.

To avoid a lot of unnecessary configuration options on SpannerOptions, the retry settings of the three underlying clients are exposed by exposing the StubSettings.Builders that are used to create the underlying clients. This only adds three extra methods to SpannerOptions, while still granting (advanced) users access to all configuration options.

The client library will use the defaults that are globally defined for retries in the configuration yaml files of Spanner (i.e. in the files spanner_gapic.yaml, spanner_admin_database_gapic.yaml and spanner_admin_instance_gapic.yaml)

The following defaults are defined:

  • Spanner default settings:
    • Initial retry delay: 1 second
    • Retry multiplier: 1.3
    • Max retry delay: 32 seconds
    • Initial RPC timeout: 60 seconds
    • RPC timeout multiplier: 1.0
    • Max RPC timeout: 60 seconds
    • Total timeout: 600 seconds
  • Spanner streaming settings (used for executeStreamingSql and streamingRead):
    • Initial retry delay: 1 second
    • Retry multiplier: 1.3
    • Max retry delay: 32 seconds
    • Initial RPC timeout: 120 seconds
    • RPC timeout multiplier: 1.0
    • Max RPC timeout: 120 seconds
    • Total timeout: 1200 seconds
  • Spanner long-running settings (used for commit):
    • Initial retry delay: 1 second
    • Retry multiplier: 1.3
    • Max retry delay: 32 seconds
    • Initial RPC timeout: 3600 seconds
    • RPC timeout multiplier: 1.0
    • Max RPC timeout: 3600 seconds
    • Total timeout: 3600 seconds
  • Database admin default settings (used for all methods except methods that return an operation, i.e. createDatabase and updateDatabaseDdl, these have a total timeout of 24 hours):
    • Initial retry delay: 1 second
    • Retry multiplier: 1.3
    • Max retry delay: 32 seconds
    • Initial RPC timeout: 60 seconds
    • RPC timeout multiplier: 1.0
    • Max RPC timeout: 60 seconds
    • Total timeout: 600 seconds
  • Instance admin default settings (used for all methods except methods that return an operation, i.e. createInstance and updateInstance, these have a total timeout of 24 hours):
    • Initial retry delay: 1 second
    • Retry multiplier: 1.3
    • Max retry delay: 32 seconds
    • Initial RPC timeout: 60 seconds
    • RPC timeout multiplier: 1.0
    • Max RPC timeout: 60 seconds
    • Total timeout: 600 seconds

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

codecov bot commented Apr 14, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4940      +/-   ##
============================================
+ Coverage     49.98%   50.31%   +0.33%     
- Complexity    23668    23676       +8     
============================================
  Files          2238     2238              
  Lines        226007   226059      +52     
  Branches      24191    24955     +764     
============================================
+ Hits         112974   113751     +777     
- Misses       103693   103707      +14     
+ Partials       9340     8601     -739
Impacted Files Coverage Δ Complexity Δ
...re/src/main/java/com/google/cloud/BaseService.java 55.55% <0%> (-6.95%) 2% <0%> (ø)
...le/cloud/compute/deprecated/DeprecationStatus.java 88.63% <0%> (-3.13%) 20% <0%> (ø)
...va/com/google/cloud/http/HttpTransportOptions.java 56.33% <0%> (-1.64%) 10% <0%> (ø)
...ge/contrib/nio/CloudStorageFileSystemProvider.java 61.26% <0%> (-0.29%) 70% <0%> (ø)
...ain/java/com/google/cloud/logging/LoggingImpl.java 88.44% <0%> (-0.2%) 73% <0%> (ø)
.../google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java 6.92% <0%> (-0.06%) 2% <0%> (ø)
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java 1.82% <0%> (-0.01%) 1% <0%> (ø)
...loud/compute/v1/InsertNodeTemplateHttpRequest.java 23.92% <0%> (ø) 9% <0%> (ø) ⬇️
...in/java/com/google/cloud/compute/v1/NodeGroup.java 38.6% <0%> (ø) 17% <0%> (ø) ⬇️
...java/com/google/cloud/compute/v1/NodeTemplate.java 36.41% <0%> (ø) 19% <0%> (ø) ⬇️
... and 147 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 2eaa9f4...28935c3. Read the comment docs.

@olavloite olavloite force-pushed the spanner-use-gax-for-retries branch from 93fa13d to 7fc30fe Compare April 16, 2019 12:33
@olavloite olavloite changed the title [WIP] Spanner: Use gax for retries Spanner: Use gax for retries Apr 17, 2019
@olavloite olavloite marked this pull request as ready for review April 17, 2019 10:15
@olavloite olavloite requested a review from a team as a code owner April 17, 2019 10:15
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 20, 2019
@olavloite olavloite force-pushed the spanner-use-gax-for-retries branch from c5cfbd5 to 615a600 Compare April 22, 2019 16:48
@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2019
@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2019
@codecov-io
Copy link

codecov-io commented May 1, 2019

Codecov Report

Merging #4940 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4940      +/-   ##
============================================
+ Coverage     50.41%   50.42%   +<.01%     
- Complexity    23796    23797       +1     
============================================
  Files          2251     2251              
  Lines        226833   226833              
  Branches      24966    24966              
============================================
+ Hits         114368   114370       +2     
+ Misses       103858   103857       -1     
+ Partials       8607     8606       -1
Impacted Files Coverage Δ Complexity Δ
...able/gaxx/reframing/ReframingResponseObserver.java 90.82% <0%> (+1.83%) 30% <0%> (+1%) ⬆️

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 db3241b...1302c06. Read the comment docs.

@sduskis sduskis added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels May 17, 2019
@olavloite olavloite force-pushed the spanner-use-gax-for-retries branch from 129a2d0 to 000f3c3 Compare May 17, 2019 13:06
@olavloite olavloite removed the needs work This is a pull request that needs a little love. label May 17, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label May 17, 2019
@olavloite
Copy link
Author

This PR will also solve the original issue mentioned in #3616, as it will enable users to configure timeouts and custom retry settings for a SpannerOptions instance.

@olavloite olavloite merged commit 2ae5ca0 into googleapis:master May 29, 2019
@olavloite olavloite deleted the spanner-use-gax-for-retries branch May 29, 2019 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants