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

chore(spanner): track precommit token for R/W multiplexed session #3411

Merged
merged 9 commits into from
Oct 18, 2024

Conversation

harshachinta
Copy link
Contributor

@harshachinta harshachinta commented Oct 15, 2024

When a read-write transaction is executed on a multiplexed session, the RPC responses of that transaction return a MultiplexedSessionPrecommitToken. In client library, the precommit token with the highest sequence number is tracked at the transaction context level. During the commit, this latest precommit token is fetched and set in the CommitRequest. If the precommit token is not set during the commit, the backend will throw an INVALID_ARGUMENT error.

Including the latest token in the CommitRequest is essential to prevent latency regression, though it does not impact the correctness of the transaction.

This PR tracks the precommit token from the following RPC responses,

  1. ResultSet
  2. PartialResultSet
  3. ExecuteBatchDmlResponse

@harshachinta harshachinta requested a review from a team as a code owner October 15, 2024 06:29
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Oct 15, 2024
@@ -2447,4 +2471,23 @@ Session getSession(String name) {
}
return null;
}

static MultiplexedSessionPrecommitToken getResultSetPrecommitToken() {
return getPrecommitToken("ResultSetPrecommitToken", 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The sequence numbers should be more intelligent than this. This just assumes that RPCs will be called in a given order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for hardcoding the sequence number is to maintain greater control in our tests and to minimize flakiness. This approach does not assume that RPCs will be called in a strict order. For instance, consider the following RPC response order in a read-write transaction:

ResultSet -> sequence number 1
PartialResultSet -> sequence number 3
ExecuteBatchDml -> sequence number 2

Even though the ExecuteBatchDml RPC was executed later, we know that the PartialResultSet, with sequence number 3, will have the latest precommit token. This ensures that we are properly validating the sequence number logic

However, I agree that there is a flaw in this approach. If the same order of RPCs is executed on the backend, the ExecuteBatchDml would have the latest sequence number since it was the last RPC executed in the transaction. In contrast, our mock Spanner does not adhere to this behavior, which causes the discrepancy.

Other options.

  1. Incremental counter - We can use AtomicInteger to generate incrementing sequence numbers in a thread-safe manner. This ensures that the seq number is assigned based on when the request reaches mock spanner. This also ensure seq number is always incremental and will not lead to flakiness.
  2. Time-Based Increments - We can use Instant.now() as sequence number. However the seq number in this case will not always be incremental (2 RPCs can have same seq number) leading to test flakiness.

I have modified code to use incremental counter.

However, please let me know if there are other better ways to solve this.

@harshachinta harshachinta added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Oct 18, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 18, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit aeeea3c into googleapis:main Oct 18, 2024
30 of 31 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 18, 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