-
Notifications
You must be signed in to change notification settings - Fork 123
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.
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
feat: add support for Directed Read options #2766
Conversation
…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206
fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
Show resolved
Hide resolved
IncludeReplicas.newBuilder() | ||
.addReplicaSelections( | ||
ReplicaSelection.newBuilder() | ||
.setLocation("us-west1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this fail if someone tries to run this integration test on an instance config that does not have a read-only replica in us-west1
? If so; could we handle that more gracefully? (Could we for example read the possible values from the actual instance config, and set one of those here?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the provided replica is not available in that instance config, then the request will silently ignore the setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olavloite Based on what @harshachinta mentioned, it should be ok with the way the tests are currently written? Or do you think we can further enhance something? I think its fine, since over here we don't have a way to assert if backend used the setting or ignored it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually makes me worry a bit more, but more generally about the feature than about the test :-)
The fact that an invalid value is silently ignored means that the user does not get any feedback about whether the feature is being used or not. If a user makes a typo for the location, then there is no feedback loop that will indicate that to the user, which again can make for very difficult debugging.
Could we file a request with the feature owner that there is some kind of feedback from the server about which replica was actually used to serve the read? Or do we already have that information in some of the metadata that we receive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayurkale22 Can you take a look here and answer this query? I think this is another reason why we should return the feedback in our backend response (similar to what we were discussing last evening)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the provided replica is not available in that instance config, then the request will silently ignore the setting.
In the earlier design, the plan was to ensure that the location and/or replica type specified in the directed reads settings does indeed belong to the instance configuration used, otherwise, return an INVALID_ARGUMENT
error to indicate users that they are using the incorrect setting/location. However, this means that if the currently specified location(s) from the source instance configuration does not exist in the destination instance configuration, this would result in requests errors during the instance move/migration process. That's a poor user experience.
To support friction-free instance migration, we will eliminate the above constraint. So basically, if the location(s) provided in directed reads settings are not found in the instance configuration, the Spanner will fallback to default routing mechanism by sending the requests to the nearest available replica and hence this will ensure that the directed read requests do not fail during/after the instance migration. To tell users about incorrect location in the directed reads, we plan to expose a new metric. The metric can be used to understand misconfigured settings.
Could we file a request with the feature owner that there is some kind of feedback from the server about which replica was actually used to serve the read?
We plan to expose this info in the metric for now. In the future we can expose in the API response itself.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITDirectedReadsTest.java
Outdated
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [6.56.0](https://togithub.com/googleapis/java-spanner/compare/v6.55.0...v6.56.0) (2024-01-05) ### Features * Add autoscaling config in the instance to support autoscaling in systests ([#2756](https://togithub.com/googleapis/java-spanner/issues/2756)) ([99ae565](https://togithub.com/googleapis/java-spanner/commit/99ae565c5e90a2862b4f195fe64656ba8a05373d)) * Add support for Directed Read options ([#2766](https://togithub.com/googleapis/java-spanner/issues/2766)) ([26c6c63](https://togithub.com/googleapis/java-spanner/commit/26c6c634b685bce66ce7caf05057a98e9cc6f5dc)) * Update OwlBot.yaml file to pull autogenerated executor code ([#2754](https://togithub.com/googleapis/java-spanner/issues/2754)) ([20562d4](https://togithub.com/googleapis/java-spanner/commit/20562d4d7e62ab20bb1c4e78547b218a9a506f21)) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.21.0 ([#2772](https://togithub.com/googleapis/java-spanner/issues/2772)) ([173f520](https://togithub.com/googleapis/java-spanner/commit/173f520f931073c4c6ddf3b3d98d255fb575914f)) ### Documentation * Samples and tests for auto-generated createDatabase and createInstance APIs. ([#2764](https://togithub.com/googleapis/java-spanner/issues/2764)) ([74a586f](https://togithub.com/googleapis/java-spanner/commit/74a586f8713ef742d65400da8f04a750316faf78)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Customers can set the option at two level a) per RPC by passing
Options.directedRead
or b) global setting inSpannerOptions
. The option is not applicable for RW transactions. If customer sets the setting for a RW operation, they will receive an exception.Sample Usage