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 BatchWriteAtLeastOnce #2520

Merged
merged 28 commits into from
Sep 26, 2023

Conversation

rajatbhatta
Copy link
Contributor

@rajatbhatta rajatbhatta commented Jul 6, 2023

No description provided.

@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 Jul 6, 2023
@rajatbhatta rajatbhatta marked this pull request as ready for review July 6, 2023 13:13
@rajatbhatta rajatbhatta requested a review from a team as a code owner July 6, 2023 13:13
@rajatbhatta rajatbhatta requested review from olavloite and arpan14 July 6, 2023 13:13
*
* @return ServerStream\<BatchWriteResponse>
*/
ServerStream<BatchWriteResponse> batchWriteAtLeastOnce(Iterable<MutationGroup> mutationGroups)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When should I create multiple MutationGroups instead of just one large mutation group? What's the tradeoff between a large number of small MutationGroups vs a few large MutationGroups? Is there any limits on the number of mutations that I may have in a MutationGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When should I create multiple MutationGroups instead of just one large mutation group?

Dependent mutations (to be committed together) should be part of a single MutationGroup.

For example, the following should be part of same MutationGroup:

  • Inserting rows into both parent and child tables. If they're part of different MutationGroups, it will result in a failure when a commit on the child table’s row is attempted first.
  • Inserting rows into tables T1, T2; T2 has a foreign key pointing to T1. If they're part of different MutationGroups, it will result in a failure when a commit on T2 is attempted first.

If there's a need to create just one large MutationGroup, it's better to use the existing Commit RPC for it.

What's the tradeoff between a large number of small MutationGroups vs a few large MutationGroups?

Depends on the use case. The bottomline is we should only put mutations meant to be committed together in a group, otherwise, should prefer a different group.

Is there any limits on the number of mutations that I may have in a MutationGroup?

Considering that in the base case, each mutation group could be committed into its own transaction, which has a limit of 40k mutations, the same limit would apply to the MutationGroup also.

@rajatbhatta rajatbhatta changed the title feat: add support for BatchWriteAtleastOnce feat: add support for BatchWriteAtLeastOnce Aug 23, 2023
@rajatbhatta rajatbhatta requested a review from olavloite August 23, 2023 07:29
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.

LGTM, with a couple of nits on the documentation. This is a non-trivial feature, so making sure that we make the documentation as clear as possible will help our users make the most out of this.

@arpan14 arpan14 added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 26, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 26, 2023
@arpan14 arpan14 merged commit 8ea7bd1 into googleapis:main Sep 26, 2023
18 of 19 checks passed
@arpan14 arpan14 deleted the batch-write-review branch September 26, 2023 05:08
surbhigarg92 pushed a commit to surbhigarg92/java-spanner that referenced this pull request Oct 5, 2023
* feat: add support for BatchWriteAtleastOnce

* test: add batchwrite() support to MockSpannerServiceImpl

* test: add commit timestamp to proto

* test: add commit timestamp to proto

* test: add commit timestamp to proto

* consume the stream in tests

* refactor tests

* refactor tests

* test if mutations are correctly applied

* null check

* skip for emulator

* add method documentation

* add method documentation

* add method documentation

* remove autogenerated code

* remove autogenerated tests

* batchWriteAtleastOnce -> batchWriteAtLeastOnce

* batchWriteAtleastOnceWithOptions -> batchWriteAtLeastOnceWithOptions

* changes based on updated batch write API

* add copyright and doc

* address review comments

* address review comments

* add more documentation

---------

Co-authored-by: Arpan Mishra <[email protected]>
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