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): set client wide ReadOptions, ApplyOptions, and TransactionOptions #6486

Merged
merged 6 commits into from
Aug 24, 2022

Conversation

shuheiktgw
Copy link
Contributor

@shuheiktgw shuheiktgw commented Aug 8, 2022

Currently, if we want to set ReadOptions, ApplyOptions, or TransactionOptions, we need to pass those options as a parameter each time we call the related functions. That is tedious and hard to maintain. Instead, I'd like to specify those options as part of the ClientConfig and use them as default values as the QueryOptions already does. Thanks for your review 🙂

@shuheiktgw shuheiktgw requested review from a team as code owners August 8, 2022 23:34
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the Spanner API. labels Aug 8, 2022
// order of precedence.
func (co CommitOptions) merge(opts CommitOptions) CommitOptions {
return CommitOptions{
ReturnCommitStats: co.ReturnCommitStats || opts.ReturnCommitStats,
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 could not figure out a good way to merge the ReturnCommitStats...

@shuheiktgw shuheiktgw force-pushed the set_client_wide_options branch 2 times, most recently from 3e73ce2 to f2ae2ad Compare August 8, 2022 23:59
@shuheiktgw shuheiktgw changed the title feat(spanner): set client wide ReadOptions and TransactionOptions feat(spanner): set client wide ReadOptions, ApplyOptions, and TransactionOptions Aug 8, 2022
@shuheiktgw
Copy link
Contributor Author

@rahul2393 Would you review this PR when you have time? 🙂

@rahul2393
Copy link
Contributor

rahul2393 commented Aug 10, 2022

@shuheiktgw Thanks for the contribution, Can you please add relevant tests for this?

@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 Aug 14, 2022
@shuheiktgw
Copy link
Contributor Author

Thanks for your comment, @rahul2393! I've added unit tests, so would you review the PR? 🙏

@rahul2393
Copy link
Contributor

Please resolve conflicts @shuheiktgw

@shuheiktgw
Copy link
Contributor Author

Sure, I've resolved the conflict so would you take a look? 🙂 @rahul2393

@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2022
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2022
@rahul2393 rahul2393 merged commit 757f1ca into googleapis:main Aug 24, 2022
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. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants