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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding support for spanner request options tags #276

Merged
merged 34 commits into from Sep 29, 2021
Merged

feat: adding support for spanner request options tags #276

merged 34 commits into from Sep 29, 2021

Conversation

vi3k6i5
Copy link
Contributor

@vi3k6i5 vi3k6i5 commented Mar 16, 2021

Adding request options to relevant methods and adding unit tests for the same.
Fixes #253 馃

@vi3k6i5 vi3k6i5 requested a review from larkee Mar 16, 2021
@vi3k6i5 vi3k6i5 requested a review from as a code owner Mar 16, 2021
@google-cla google-cla bot added the cla: yes label Mar 16, 2021
@product-auto-label product-auto-label bot added the api: spanner label Mar 16, 2021
@vi3k6i5 vi3k6i5 changed the title Spanner request options 2 feat: adding support for spanner request options tags Mar 16, 2021
@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Mar 23, 2021

System tests are failure is expected because of pending feature release.

Added sleep time of 200 seconds before looking up the tags in db. Just for validation it's okay for now, this will have to be fixed before we merge with mainline.

@vi3k6i5 vi3k6i5 requested review from skuruppu and thiagotnunes Apr 7, 2021
@larkee larkee added the do not merge label Apr 8, 2021
tests/system/test_system.py Outdated Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
Copy link
Contributor

@larkee larkee left a comment

Added some comments with more details on transactions.

Additionally, we discussed adding validation but had difficulties due to class inheritance. I think if we add a _read_only attribute to _SnapshotBase and Transaction which is set to True and False respectively, similar to _multi_use, then we should be able to add validation for the request_options.

google/cloud/spanner_v1/batch.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/snapshot.py Outdated Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
@skuruppu skuruppu requested a review from larkee Jul 21, 2021
@snippet-bot
Copy link

@snippet-bot snippet-bot bot commented Aug 27, 2021

No region tags are edited in this PR.

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

@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Aug 27, 2021

Added some comments with more details on transactions.

Additionally, we discussed adding validation but had difficulties due to class inheritance. I think if we add a _read_only attribute to _SnapshotBase and Transaction which is set to True and False respectively, similar to _multi_use, then we should be able to add validation for the request_options.

Made these changes as well.

@vi3k6i5 vi3k6i5 requested a review from larkee Sep 10, 2021
@vi3k6i5 vi3k6i5 requested a review from thiagotnunes Sep 24, 2021
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Sep 24, 2021

@thiagotnunes I ran out of time to review this before I go on vacation. Would you please be able to review and approve on Monday?

larkee
larkee approved these changes Sep 29, 2021
@larkee larkee removed the do not merge label Sep 29, 2021
@larkee larkee merged commit e16f376 into googleapis:main Sep 29, 2021
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants