Repository navigation
Spanner: Adjust timeouts for CreateDatabase and retries for other methods #5544
Description
As part of a recent change to GAPIC Configuration, timeouts and retries need to be updated.
Ensure that the timeouts and retires specified in googleapis/googleapis@cc23354#diff-4501db3e3507bcce8496c9aea2017d96 are reflected in this client library.
Activity
olavloite commentedon Jun 25, 2019
These values are part of the generated gapic client and cannot (or should not) be changed manually. Instead, the gapic client should be re-generated (and I do not have the power to do so).
@kolea2 Is this something you could initiate?
kolea2 commentedon Jun 25, 2019
I'll assign to myself and take a look!
kolea2 commentedon Jun 25, 2019
@olavloite, #5523 will resolve the retry values (the PR was failing due to testing the default values which I've just fixed). Once merged, we still need to investigate the
timeout_millisfield, as it looks like that wasn't picked up automatically.olavloite commentedon Jun 26, 2019
What is the intention of the
timeout_millisfield? To set the total timeout for the call? Because as far as I can tell, that should be the fieldtotal_timeout_millis.crwilcox commentedon Jun 27, 2019
Java has constants controlling some of this at this time. It needs to use the configurations that are generated.
kolea2 commentedon Jul 2, 2019
@olavloite looks like .net has the same issue we do: googleapis/google-cloud-dotnet#3165
olavloite commentedon Jul 2, 2019
@crwilcox @kolea2 The constants are used in two places in the Java library:
google-cloud-java/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
Line 920 in e3dd67e
google-cloud-java/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerRetryHelper.java
Line 39 in e3dd67e
(The latter is not literally using the constant, but I'll change that)
All other retries use the gapic configuration. I'll change the MIN_BACKOFF_MS value now from 1000 to 250 to conform with the gapic config.
The question is whether we want to change this from using a constant value to actually using a value from the gapic configuration as discussed here: https://github.com/googleapis/google-cloud-java/issues/5543#issuecomment-505444950
That way, this value could also dynamically change with the gapic config, but we would need to choose a config item to link it to.
olavloite commentedon Jul 2, 2019
@crwilcox
Regarding the high timeout value for CreateDatabase that is not picked up by the regen. I'm not very familiar with the gapic generator and corresponding config, but I noticed the following:
defaultthat is assigned to theCreateDatabasecall.initial_rpc_timeout_millisandtotal_timeout_millisin the default retry config, but there is no value defined fortimeout_millisin the default config.timeout_millisfor theCreateDatabasecall. This field is seemingly not picked up by the generator.Are you sure that it isn't one of the fields
initial_rpc_timeout_millisortotal_timeout_millisthat should have been overridden in the configuration for theCreateDatabasecall?yihanzhen commentedon Jul 18, 2019
This field is only used for non-retrying methods, i.e., methods where
retry_params_nameis not specified. Whenretry_params_nameis given, the method will always retry, and will follow the timeout settings defined here.To make this method not retry and respect this field as the total timeout, please remove the
retry_params_namefield.To have this method continue to retry but just change the total timeout, please add a new
retry_param_defwith longer timeout, and reference that here.skuruppu commentedon Jan 2, 2020
I believe we have already adjusted the timeouts in googleapis/spanner_gapic.yaml to what we want these to be. I'll close this issue and re-open if we need to make further adjustments to the timeouts and retries.