googleapis / java-spanner

Apache License 2.0
64 stars 116 forks source link

Administrative retry settings are not modifiable #2454

Closed Sourc closed 1 year ago

Sourc commented 1 year ago

Hi, I noticed that the administrative retry settings used for the com.google.cloud.Spanner.DatabaseAdminClient#updateDatabaseDdl method are not exposed. The retry settings used seem to only be modifiable from here.

I presume this is an oversight, unless there's another hidden way to change them that I've missed.

olavloite commented 1 year ago

This is intentional. Those settings are only used specifically for the case that the limit for the number of administrative requests has been reached: https://github.com/googleapis/java-spanner/blob/b75680860fef08e102b8370df3691ba6c29b26e0/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java#L629

This is an internal retry loop and hence these settings are also intentionally internal. If you for example were to reduce the retry backoff of these settings, you would probably end up making your system less responsive, as the more frequent retries would just cause the additional requests to cause the limit to be exceeded all the time.

Sourc commented 1 year ago

I presume a lot of settings can cause things to behave sub optimally in different situations, I don't see why these settings should be hidden - seeing as they affect functionality just as much as other retry settings, and making them modifiable wouldn't cause any issues, as long as the default settings are unchanged. We have a need to disable these and manage the quota exceeded retries ourselves, which we unfortunately can't do without ugly workarounds now.

In our case, we have a bunch of updateDdl calls coming from a lot of different servers, but often using the same operation ID. If the default updateDdl implemention retried using for example getOperation instead of just trying to issue the initial request every time, we would be fine as well (since that quota is larger) - but since the current implementation doesn't we need to manually handle it for now.