Skip to content

feat(spanner): support commit options in mutation operations.#10668

Merged
rahul2393 merged 7 commits intomainfrom
support_commit_opts
Aug 14, 2024
Merged

feat(spanner): support commit options in mutation operations.#10668
rahul2393 merged 7 commits intomainfrom
support_commit_opts

Conversation

@rahul2393
Copy link
Copy Markdown
Contributor

No description provided.

@rahul2393 rahul2393 requested review from a team August 12, 2024 05:29
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Aug 12, 2024
@rahul2393 rahul2393 force-pushed the support_commit_opts branch from 94e351d to 93c6da5 Compare August 12, 2024 05:51
Comment on lines +949 to +952
var commitOptions CommitOptions
if ao.commitOptions != nil {
commitOptions = *ao.commitOptions
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't quite understand why this is different depending on whether ao.atLeastOnce is true or not.

Comment on lines +1862 to +1863
// maxCommitDelay is the maximum time that the commit will be delayed by the
// backend before it is acknowledged.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(The field here is not maxCommitDelay, but commitOptions)

Suggested change
// maxCommitDelay is the maximum time that the commit will be delayed by the
// backend before it is acknowledged.
// commitOptions are applied to the Commit request for the writeOnlyTransaction.

Insert("Accounts", []string{"AccountId", "Nickname", "Balance"}, []interface{}{int64(2), "Bar", int64(1)}),
}
if _, err := client.Apply(ctx, accounts, ApplyAtLeastOnce()); err != nil {
duration, err := time.ParseDuration("100ms")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we also add mock server tests for both normal read/write transactions and writeAtLeastOnce transactions that verify that the maxCommitDelay that has been set is actually sent to Spanner in both cases? This integration test only shows that setting the option does not break anything, it is not able to determine whether the option was actually sent or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the tests

@rahul2393 rahul2393 requested a review from olavloite August 14, 2024 10:03
@rahul2393 rahul2393 merged commit 62a56f9 into main Aug 14, 2024
@rahul2393 rahul2393 deleted the support_commit_opts branch August 14, 2024 10:19
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.

2 participants