Skip to content
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

Closed
wants to merge 19 commits into from
Closed

Conversation

rajatbhatta
Copy link
Contributor

@rajatbhatta rajatbhatta commented Apr 10, 2023

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.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/java-spanner API. labels Apr 10, 2023
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 10, 2023
@rajatbhatta rajatbhatta marked this pull request as ready for review April 21, 2023 10:13
@rajatbhatta rajatbhatta requested a review from a team as a code owner April 21, 2023 10:13
Copy link
Collaborator

@olavloite olavloite left a 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.

DirectedReadOptions directedReadOptionsForClient,
DirectedReadOptions directedReadOptionsForRequest,
boolean readOnly) {
if (!readOnly) {
Copy link
Collaborator

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) .

Copy link
Collaborator

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:

  1. If the configuration is set at the client level (so configure once for all transactions executed by this client): This check should be removed.
  2. If the option is set at the request level, which for a read/write transaction would mean something like:
    1. Start read/write transaction
    2. Execute a query with a DirectedReadOption set.
    3. 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.

Copy link
Collaborator

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.

@arpan14
Copy link
Collaborator

arpan14 commented Jan 3, 2024

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 GapicSpannerRPC file.

@arpan14 arpan14 closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants