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(spanner): support multiplexed session for Partitioned operations #3231

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

pratickchokhani
Copy link
Contributor

@pratickchokhani pratickchokhani commented Aug 1, 2024

With multiplexed sessions, the client optimises and runs multiple applicable requests concurrently on a single session. A single multiplexed session is sufficient to handle all concurrent partitioned read, query and dml operations.

This can be enabled through an environment variable GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS_PARTITIONED_OPS.

@pratickchokhani pratickchokhani requested a review from a team as a code owner August 1, 2024 10:35
@pratickchokhani pratickchokhani marked this pull request as draft August 1, 2024 10:35
@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 Aug 1, 2024
@pratickchokhani pratickchokhani changed the title feat(spanner): support multiplexed session for Partitioned read or query feat(spanner): support multiplexed session for Partitioned operations Aug 2, 2024
Copy link

Warning: This pull request is touching the following templated files:

  • .github/workflows/ci.yaml

@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 Nov 25, 2024
@pratickchokhani pratickchokhani marked this pull request as ready for review November 25, 2024 14:52
@pratickchokhani pratickchokhani requested a review from a team as a code owner November 25, 2024 14:52
SessionImpl session = sessionClient.createSession();
SessionImpl session;
if (isMultiplexedSessionEnabled) {
session = sessionClient.createMultiplexedSession();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not make more sense to use the multiplexed session that we have already created for the normal client? Or otherwise at least just create one multiplexed session that is re-used for all batch read-only transactions? The reason that the current implementation creates a separate session for each transaction is that:

  1. We don't want to use a session from the pool, because the session might be checked out for a very long time.
  2. Regular sessions cannot be used for multiple concurrent transactions (although this restriction in reality has been lifted).

}

/** Returns a {@link SessionImpl} that references the existing session with the given name. */
SessionImpl sessionWithId(String name, boolean isMultiplexedSession) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a boolean that indicates whether the session is a multiplexed session or not here, as the caller does not know. The session might have been created by a different client, and this client does not know whether the other client created a regular or a multiplexed session.

sessionClient.sessionWithId(checkNotNull(batchTransactionId).getSessionId());
sessionClient.sessionWithId(
checkNotNull(batchTransactionId).getSessionId(),
batchTransactionId.isMultiplexedSession());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not pass in multiplexed true/false here, because this client does not know anything about this session. The session might have been created by a different client and shared out-of-band with this client. The fact that this client uses multiplexed sessions does not mean that the other client uses multiplexed sessions (and vice-versa).

@@ -32,12 +32,18 @@ public class BatchTransactionId implements Serializable {
private final ByteString transactionId;
private final String sessionId;
private final Timestamp timestamp;
private final boolean isMultiplexedSession;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this flag for anything? Otherwise, I think we should remove it. See also my comment above; If a client receives a session ID from another client, then this client does not know whether that session ID is for a regular or a multiplexed session. If such an ID is then used to create a BatchTransactionId, then this could lead to a BatchTransactionId that contains 'false' information.

pratickchokhani and others added 3 commits November 27, 2024 12:58
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Nov 27, 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: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants