-
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 #2373
Conversation
24d9343
to
f284448
Compare
…ons logic to SpannerUtil.java
…s or SpannerOptions.
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.
Do we want to support this in the JDBC driver and other related libraries as well? If so, we would need to add support for it to the Connection API.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java
Outdated
Show resolved
Hide resolved
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/SpannerOptions.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerUtil.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerUtil.java
Outdated
Show resolved
Hide resolved
DirectedReadOptions directedReadOptionsForClient, | ||
DirectedReadOptions directedReadOptionsForRequest, | ||
boolean readOnly) { | ||
if (!readOnly) { |
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.
This will end up failing all the RW transactions if the directed read options is set for the client. IMO that shouldn't be the right behaviour. Instead this setting should be silently ignored for RW transactions and considered only for RO transactions.
@olavloite WDYT?
If we avoid adding this validation (and avoid exception) apart from improving the customer experience, we will also simplify a lot of things done in this PR (for ex - introducing readOnly
across multiple method interfaces) .
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.
I don't remember exactly how this feature works, but in general I would say:
- If the configuration is set at the client level (so configure once for all transactions executed by this client): This check should be removed.
- If the option is set at the request level, which for a read/write transaction would mean something like:
- Start read/write transaction
- Execute a query with a
DirectedReadOption
set. - Then we should fail at this point instead of silently ignoring, as the user is explicitly trying to use a feature with a read/write transaction that is not supported in that case. Silently ignoring might cause the user to believe that the feature is working.
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.
The configuration can be set at both the client as well as request level. The flexibility is provided at client level so that it can be treated as a global option and avoid the need to set it per RPC. If custom sets the config both at client as well as request, then the option at client is ignored in favour of the option set at the request.
Closing this PR in favor of #2766 . I tried re-using this PR but there are too many merge conflicts and now we are using a new approach to resolve the default options (client + RPC level option provided) instead of doing at at |
This PR contains implementation, unit tests and integration tests for supporting DirectedReads in Cloud Spanner Java Client.
DirectedReads feature enables customers to load-balance their workload across multiple regions and to achieve better/uniform CPU utilization for the multi-region instance config.