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: add support for custom timeout and retry parameters in execute_update method in transactions #251

Merged
merged 7 commits into from Mar 11, 2021
Merged

feat: add support for custom timeout and retry parameters in execute_update method in transactions #251

merged 7 commits into from Mar 11, 2021

Conversation

vi3k6i5
Copy link
Contributor

@vi3k6i5 vi3k6i5 commented Mar 2, 2021

feat: Added support for custom timeout and retry parameters in transactions.

Fixes #71 馃

@vi3k6i5 vi3k6i5 requested a review from as a code owner Mar 2, 2021
@product-auto-label product-auto-label bot added the api: spanner label Mar 2, 2021
@google-cla google-cla bot added the cla: yes label Mar 2, 2021
@vi3k6i5 vi3k6i5 changed the title feat: Added support for custom timeout and retry parameters in transactions. feat: Added support for custom timeout and retry parameters in transactions Mar 2, 2021
@vi3k6i5 vi3k6i5 requested review from larkee, zoercai and olavloite Mar 3, 2021
Copy link

@olavloite olavloite left a comment

This change in itself seems reasonable to me, but I don't know the exact background for this change. From the description of the PR it seems that the goal is to add custom timeout and retry parameters for transactions. This PR only introduces timeout and retry parameters for single update statements in a transaction. It does not:

  1. Add the possibility to specify a timeout for Begin, Commit or ExecuteBatchDML statements in a transaction.
  2. Add the possibility to set a timeout for the entire transaction (which I would also expect not to be something we want, but I'm a little confused by the PR title)

Is this intentional?

@vi3k6i5 vi3k6i5 changed the title feat: Added support for custom timeout and retry parameters in transactions feat: Added support for custom timeout and retry parameters in execute_update method in transactions. Mar 3, 2021
@vi3k6i5
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 commented Mar 3, 2021

Thanks @olavloite . I Updated the PR title, that should clarify it. We were planning to add this parameter 1 by 1 in other places as well. @larkee : Shall I update the same PR with other implementations as well?

@vi3k6i5 vi3k6i5 changed the title feat: Added support for custom timeout and retry parameters in execute_update method in transactions. feat: Added support for custom timeout and retry parameters in execute_update method in transactions Mar 3, 2021
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Mar 3, 2021

I think this is fine since we may want to see how this looks for one method first. Then in the next PR, we can do multiples of these.

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

This looks good to me, but I have a few questions:

  1. Are we going to add custom timeouts for other methods as well?
  2. Are we also planning on making it possible to override the default retry strategy of the methods?
  3. Do we want to provide a way so that a timeout is set during initialization and the caller does not have to specify the timeout on every call (or do we already have this functionality)?

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Mar 5, 2021

Sorry, I missed the above comments, so I think it looks good, since this is the first step of the feature.

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Mar 5, 2021

Please wait for @larkee 's review before merging, as they have more context on the library implementation.

google/cloud/spanner_v1/transaction.py Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Outdated Show resolved Hide resolved
@vi3k6i5 vi3k6i5 requested a review from larkee Mar 10, 2021
@larkee larkee added the kokoro:force-run label Mar 10, 2021
@larkee larkee changed the title feat: Added support for custom timeout and retry parameters in execute_update method in transactions feat: add support for custom timeout and retry parameters in execute_update method in transactions Mar 10, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Mar 10, 2021
larkee
larkee approved these changes Mar 10, 2021
Copy link
Contributor Author

@vi3k6i5 vi3k6i5 left a comment

Build check failed, retrying.

@vi3k6i5 vi3k6i5 merged commit 8abaebd into googleapis:master Mar 11, 2021
8 checks passed
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 25, 2021
馃 I have created a release \*beep\* \*boop\*
---
## [3.3.0](https://www.github.com/googleapis/python-spanner/compare/v3.2.0...v3.3.0) (2021-03-25)


### Features

* add encryption_info to Database ([#284](https://www.github.com/googleapis/python-spanner/issues/284)) ([2fd0352](https://www.github.com/googleapis/python-spanner/commit/2fd0352f695d7ab85e57d8c4388f42f91cf39435))
* add support for CMEK ([#105](https://www.github.com/googleapis/python-spanner/issues/105)) ([e990ff7](https://www.github.com/googleapis/python-spanner/commit/e990ff70342e7c2e27059e82c8d74cce39eb85d0))
* add support for custom timeout and retry parameters in execute_update method in transactions ([#251](https://www.github.com/googleapis/python-spanner/issues/251)) ([8abaebd](https://www.github.com/googleapis/python-spanner/commit/8abaebd9edac198596e7bd51d068d50147d0391d))
* added retry and timeout params to partition read in database and snapshot class ([#278](https://www.github.com/googleapis/python-spanner/issues/278)) ([1a7c9d2](https://www.github.com/googleapis/python-spanner/commit/1a7c9d296c23dfa7be7b07ea511a4a8fc2c0693f))
* **db_api:** support executing several DDLs separated by semicolon ([#277](https://www.github.com/googleapis/python-spanner/issues/277)) ([801ddc8](https://www.github.com/googleapis/python-spanner/commit/801ddc87434ff9e3c86b1281ebfeac26195c06e8))


### Bug Fixes

* avoid consuming pending null values when merging ([#286](https://www.github.com/googleapis/python-spanner/issues/286)) ([c6cba9f](https://www.github.com/googleapis/python-spanner/commit/c6cba9fbe4c717f1f8e2a97e3f76bfe6b956e55b))
* **db_api:** allow file path for credentials ([#221](https://www.github.com/googleapis/python-spanner/issues/221)) ([1de0284](https://www.github.com/googleapis/python-spanner/commit/1de028430b779a50d38242fe70567e92b560df5a))
* **db_api:** ensure DDL statements are being executed ([#290](https://www.github.com/googleapis/python-spanner/issues/290)) ([baa02ee](https://www.github.com/googleapis/python-spanner/commit/baa02ee1a352f7c509a3e169927cf220913e521f))
* **db_api:** revert Mutations API usage ([#285](https://www.github.com/googleapis/python-spanner/issues/285)) ([e5d4901](https://www.github.com/googleapis/python-spanner/commit/e5d4901e9b7111b39dfec4c56032875dc7c6e74c))


### Documentation

* fix docstring types and typos ([#259](https://www.github.com/googleapis/python-spanner/issues/259)) ([1b0ce1d](https://www.github.com/googleapis/python-spanner/commit/1b0ce1d2986085ce4033cf773eb6c5d3b904473c))
* fix snapshot usage ([#291](https://www.github.com/googleapis/python-spanner/issues/291)) ([eee2181](https://www.github.com/googleapis/python-spanner/commit/eee218164c3177586b73278aa21495280984af89))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.

6 participants