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): single-RPC, batched commit of mutation groups #12930

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

devbww
Copy link
Contributor

@devbww devbww commented Oct 19, 2023

spanner::Client::CommitAtLeastOnce(std::vector<Mutations>)

Add support for committing mutation groups, batched efficiently into transactions with at-least-once semantics, using a single RPC.

All mutations within a group are committed atomically, but there is no atomicity or ordering between groups, so they must be independent of each other.

Partial failure is possible. That is, some batches can commit while others fail. The results of individual batches are returned via the response stream as their transactions complete.

Mutation groups are not replay protected. That is, it is possible that any mutation group may be applied more than once. They should be idempotent to avoid replay complications.


This change is Reviewable

`spanner::Client::CommitAtLeastOnce(std::vector<Mutations>)`

Add support for committing mutation groups, batched efficiently into
transactions with at-least-once semantics, using a single RPC.

All mutations within a group are committed atomically, but there is
no atomicity or ordering between groups, so they must be independent
of each other.

Partial failure is possible. That is, some batches can commit while
others fail. The results of individual batches are returned via the
response stream as their transactions complete.

Mutation groups are not replay protected. That is, it is possible
that any mutation group may be applied more than once. They should
be idempotent to avoid replay complications.
@devbww devbww temporarily deployed to internal October 19, 2023 18:31 — with GitHub Actions Inactive
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Attention: 98 lines in your changes are missing coverage. Please review.

Comparison is base (07ecb92) 93.62% compared to head (b4a9526) 93.58%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12930      +/-   ##
==========================================
- Coverage   93.62%   93.58%   -0.04%     
==========================================
  Files        2067     2067              
  Lines      180220   180498     +278     
==========================================
+ Hits       168726   168925     +199     
- Misses      11494    11573      +79     
Files Coverage Δ
google/cloud/spanner/client.cc 98.15% <100.00%> (+0.04%) ⬆️
google/cloud/spanner/client.h 100.00% <ø> (ø)
google/cloud/spanner/connection.h 100.00% <ø> (ø)
google/cloud/spanner/internal/connection_impl.h 100.00% <ø> (ø)
...gle/cloud/spanner/internal/connection_impl_test.cc 98.31% <100.00%> (+0.03%) ⬆️
...ogle/cloud/spanner/mocks/mock_spanner_connection.h 70.58% <100.00%> (+1.83%) ⬆️
google/cloud/spanner/connection.cc 0.00% <0.00%> (ø)
google/cloud/spanner/client_test.cc 95.42% <92.18%> (-0.28%) ⬇️
google/cloud/spanner/internal/connection_impl.cc 93.67% <83.92%> (-0.86%) ⬇️
google/cloud/spanner/samples/samples.cc 58.14% <3.70%> (-0.55%) ⬇️
... and 1 more

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devbww devbww marked this pull request as ready for review October 19, 2023 19:26
@devbww devbww requested a review from a team as a code owner October 19, 2023 19:26
@snippet-bot
Copy link

snippet-bot bot commented Oct 19, 2023

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

Copy link
Member

@alevenberg alevenberg left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't know much about the spanner API, so might want to get a second set of eyes before submitting

@@ -307,13 +307,22 @@ StatusOr<CommitResult> Client::Commit(Transaction transaction,
StatusOr<CommitResult> Client::CommitAtLeastOnce(
Transaction::ReadWriteOptions transaction_options, Mutations mutations,
Options opts) {
// Note: This implementation differs from `CommitAtLeastOnce({mutations})`
// for historical reasons.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a link explaining the differences that might be helpful. Leave it alone if it is just "path dependent"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know of no such explanation. Indeed, I'm only intimating that the existing CommitAtLeastOnce(mutations) and the new CommitAtLeastOnce({mutations}) must behave the same (modulo the former being able to return CommitStats) because anything else would be crazy. If they had thought of the new batching interface first, then I imagine the single-mutations call would never exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave as-is then.

@@ -634,6 +634,43 @@ class Client {
Transaction::ReadWriteOptions transaction_options, Mutations mutations,
Options opts = {});

/**
* Commits the mutation groups, batched efficiently into transactions with
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clear documentation.


/// If OK, the Cloud Spanner timestamp at which the transaction committed,
/// and otherwise the reason why the commit failed.
StatusOr<Timestamp> commit_timestamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking to myself: I wonder if we really need a streaming API in the library. I know the proto is streaming, but maybe we can simply collect all the results and return all of them. Hmmm... there is the problem of blocking until they all arrive. Sold.

@devbww devbww temporarily deployed to internal October 20, 2023 16:54 — with GitHub Actions Inactive
@devbww devbww merged commit 11e9d0f into googleapis:main Oct 20, 2023
54 checks passed
@devbww devbww deleted the batch-write-2 branch October 20, 2023 17:49
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 Spanner API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants