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

fix: add ConnectionError to default retry #445

Merged
merged 8 commits into from May 20, 2021
Merged

Conversation

cojenco
Copy link
Contributor

@cojenco cojenco commented May 17, 2021

This adds python built-in ConnectionError to default retryable types.
ConnectionError was recently added in the BigQuery library to allow retries.

Fixes #426 🦕

@cojenco cojenco requested a review from a team May 17, 2021 17:45
@cojenco cojenco requested a review from a team as a code owner May 17, 2021 17:45
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label May 17, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 17, 2021
@cojenco cojenco marked this pull request as draft May 18, 2021 04:33
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

thanks!

@@ -22,6 +22,7 @@


_RETRYABLE_TYPES = (
ConnectionError,
Copy link
Contributor

Choose a reason for hiding this comment

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

ConnectionError has a bunch of subclasses: https://docs.python.org/3/library/exceptions.html#ConnectionError . Just wanted to confirm that we consider all of these retryable?

Also, is there a test where we should add this as a test case?

Copy link
Contributor

@tseaver tseaver May 18, 2021

Choose a reason for hiding this comment

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

One or more tests should be added to the Test_should_retry class in tests/unit/test_retry.py (see the BigQuery PR for examples).

Also, that PR makes requests.exceptions.ConnectionError an explicit retryable type. The requests version does not derive from the stdlib type:

$ .nox/unit-3-9/bin/python
Python 3.9.0 (default, Apr 20 2021, 11:50:03) 
[GCC 9.3.0] on linux
>>> import requests.exceptions
>>> requests.exceptions.ConnectionError
<class 'requests.exceptions.ConnectionError'>
>>> requests.exceptions.ConnectionError.__mro__
(<class 'requests.exceptions.ConnectionError'>, <class 'requests.exceptions.RequestException'>, <class 'OSError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that ConnectionError is not a stdlib type in Python 2.7:

$ .nox/unit-2-7/bin/python
Python 2.7.17 (default, Apr 20 2021, 14:27:04) 
[GCC 9.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> ConnectionError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'ConnectionError' is not defined

We need to do something like:

try:
    _RETRYABLE_STDLIB_TYPES = {ConnectionError,)
except NameError:
    _RETRYABLE_STDLIB_TYPES = {}

and then, below:

RETRYABLE_TYPES = _RETRYABLE_STDLIB_TYPES + (
    api_exceptions.TooManyRequests,  # 429
    ...
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tritone @tseaver Thanks for reviewing! Looking at the python3 docs and previous discussion in the Big Query, ConnectionError and its subclasses are retryable. From users feedback, this error occurs when there are multiple Cloud Functions running and/or subsequent reinvocation of Cloud Functions. The retry test is set up with a loop that tests each default retry. I'll double check to make the changes and add one or more specific tests. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tseaver I noticed that ConnectionError is not a stdlib type in Python 2.7 upon submission. Changed PR to draft to implement changes. This is exactly what I need. Thanks so much for the pointers!

@@ -22,6 +22,7 @@


_RETRYABLE_TYPES = (
ConnectionError,
Copy link
Contributor

@tseaver tseaver May 18, 2021

Choose a reason for hiding this comment

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

One or more tests should be added to the Test_should_retry class in tests/unit/test_retry.py (see the BigQuery PR for examples).

Also, that PR makes requests.exceptions.ConnectionError an explicit retryable type. The requests version does not derive from the stdlib type:

$ .nox/unit-3-9/bin/python
Python 3.9.0 (default, Apr 20 2021, 11:50:03) 
[GCC 9.3.0] on linux
>>> import requests.exceptions
>>> requests.exceptions.ConnectionError
<class 'requests.exceptions.ConnectionError'>
>>> requests.exceptions.ConnectionError.__mro__
(<class 'requests.exceptions.ConnectionError'>, <class 'requests.exceptions.RequestException'>, <class 'OSError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>)

@@ -22,6 +22,7 @@


_RETRYABLE_TYPES = (
ConnectionError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that ConnectionError is not a stdlib type in Python 2.7:

$ .nox/unit-2-7/bin/python
Python 2.7.17 (default, Apr 20 2021, 14:27:04) 
[GCC 9.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> ConnectionError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'ConnectionError' is not defined

We need to do something like:

try:
    _RETRYABLE_STDLIB_TYPES = {ConnectionError,)
except NameError:
    _RETRYABLE_STDLIB_TYPES = {}

and then, below:

RETRYABLE_TYPES = _RETRYABLE_STDLIB_TYPES + (
    api_exceptions.TooManyRequests,  # 429
    ...
)

@cojenco cojenco marked this pull request as ready for review May 19, 2021 16:56
@cojenco cojenco requested review from tseaver and tritone May 19, 2021 17:23
tests/unit/test_retry.py Outdated Show resolved Hide resolved
tests/unit/test_retry.py Show resolved Hide resolved
tests/unit/test_retry.py Outdated Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented May 19, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 19, 2021
@google-cla
Copy link

google-cla bot commented May 19, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@tseaver
Copy link
Contributor

tseaver commented May 19, 2021

@googlebot I fixed it.

@google-cla
Copy link

google-cla bot commented May 19, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@cojenco
Copy link
Contributor Author

cojenco commented May 19, 2021

@googlebot I fixed it.

@google-cla
Copy link

google-cla bot commented May 19, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@tseaver
Copy link
Contributor

tseaver commented May 19, 2021

@cojenco I don't know what the CLAbot is on about. As a Googler, I believe you can manually clear the cla: no label and set cla: yes instead.

@cojenco cojenco added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 19, 2021
@cojenco
Copy link
Contributor Author

cojenco commented May 19, 2021

Thanks @tseaver! Manually added the cla label. I'm looking at my configs. So when I commited the suggestion directly on Github, the email logged was a <users.noreply.github.com> account.

@cojenco cojenco added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 19, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 19, 2021
@tseaver
Copy link
Contributor

tseaver commented May 19, 2021

Ugh, I tested that syntax at the Python command prompt, where __buitins__ is a module. Per the Python docs for the builtins module, we should probably not rely on it being either the module or its dict, and instead move to something like:

try:
    ConnectionError
except NameError:
    _HAS_CONNECTION_ERROR = False
else:
    _HAS_CONNECTION_ERROR = True

and then use that flag to control the skip:

    @unittest.skipUnless(_HAS_CONNECTION_ERROR, "...")

Since my suggestion didn't work out, and messed up the CLAbot, maybe it would be best to rebase away
bcfe2be and start fresh.

renovate-bot and others added 2 commits May 19, 2021 15:25
…444)

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [pre-commit/pre-commit-hooks](https://togithub.com/pre-commit/pre-commit-hooks) | repository | major | `v3.4.0` -> `v4.0.1` |

---

### Release Notes

<details>
<summary>pre-commit/pre-commit-hooks</summary>

### [`v4.0.1`](https://togithub.com/pre-commit/pre-commit-hooks/releases/v4.0.1)

[Compare Source](https://togithub.com/pre-commit/pre-commit-hooks/compare/v4.0.0...v4.0.1)

##### Fixes

-   `check-shebang-scripts-are-executable` fix entry point.
    -   [#&#8203;602](https://togithub.com/pre-commit/pre-commit-hooks/issues/602) issue by [@&#8203;Person-93](https://togithub.com/Person-93).
    -   [#&#8203;603](https://togithub.com/pre-commit/pre-commit-hooks/issues/603) PR by [@&#8203;scop](https://togithub.com/scop).

### [`v4.0.0`](https://togithub.com/pre-commit/pre-commit-hooks/releases/v4.0.0)

[Compare Source](https://togithub.com/pre-commit/pre-commit-hooks/compare/v3.4.0...v4.0.0)

##### Features

-   `check-json`: report duplicate keys.
    -   [#&#8203;558](https://togithub.com/pre-commit/pre-commit-hooks/issues/558) PR by [@&#8203;AdityaKhursale](https://togithub.com/AdityaKhursale).
    -   [#&#8203;554](https://togithub.com/pre-commit/pre-commit-hooks/issues/554) issue by [@&#8203;adamchainz](https://togithub.com/adamchainz).
-   `no-commit-to-branch`: add `main` to default blocked branches.
    -   [#&#8203;565](https://togithub.com/pre-commit/pre-commit-hooks/issues/565) PR by [@&#8203;ndevenish](https://togithub.com/ndevenish).
-   `check-case-conflict`: check conflicts in directory names as well.
    -   [#&#8203;575](https://togithub.com/pre-commit/pre-commit-hooks/issues/575) PR by [@&#8203;slsyy](https://togithub.com/slsyy).
    -   [#&#8203;70](https://togithub.com/pre-commit/pre-commit-hooks/issues/70) issue by [@&#8203;andyjack](https://togithub.com/andyjack).
-   `check-vcs-permalinks`: forbid other branch names.
    -   [#&#8203;582](https://togithub.com/pre-commit/pre-commit-hooks/issues/582) PR by [@&#8203;jack1142](https://togithub.com/jack1142).
    -   [#&#8203;581](https://togithub.com/pre-commit/pre-commit-hooks/issues/581) issue by [@&#8203;jack1142](https://togithub.com/jack1142).
-   `check-shebang-scripts-are-executable`: new hook which ensures shebang'd scripts are executable.
    -   [#&#8203;545](https://togithub.com/pre-commit/pre-commit-hooks/issues/545) PR by [@&#8203;scop](https://togithub.com/scop).

##### Fixes

-   `check-executables-have-shebangs`: Short circuit shebang lookup on windows.
    -   [#&#8203;544](https://togithub.com/pre-commit/pre-commit-hooks/issues/544) PR by [@&#8203;scop](https://togithub.com/scop).
-   `requirements-txt-fixer`: Fix comments which have indentation
    -   [#&#8203;549](https://togithub.com/pre-commit/pre-commit-hooks/issues/549) PR by [@&#8203;greshilov](https://togithub.com/greshilov).
    -   [#&#8203;548](https://togithub.com/pre-commit/pre-commit-hooks/issues/548) issue by [@&#8203;greshilov](https://togithub.com/greshilov).
-   `pretty-format-json`: write to stdout using UTF-8 encoding.
    -   [#&#8203;571](https://togithub.com/pre-commit/pre-commit-hooks/issues/571) PR by [@&#8203;jack1142](https://togithub.com/jack1142).
    -   [#&#8203;570](https://togithub.com/pre-commit/pre-commit-hooks/issues/570) issue by [@&#8203;jack1142](https://togithub.com/jack1142).
-   Use more inclusive language.
    -   [#&#8203;599](https://togithub.com/pre-commit/pre-commit-hooks/issues/599) PR by [@&#8203;asottile](https://togithub.com/asottile).

##### Breaking changes

-   Remove deprecated hooks: `flake8`, `pyflakes`, `autopep8-wrapper`.
    -   [#&#8203;597](https://togithub.com/pre-commit/pre-commit-hooks/issues/597) PR by [@&#8203;asottile](https://togithub.com/asottile).

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻️ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/python-storage).
@cojenco cojenco requested a review from tseaver May 19, 2021 23:53
@tseaver tseaver added the automerge Merge the pull request once unit tests and other checks pass. label May 20, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 8344253 into master May 20, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the connection-426 branch May 20, 2021 15:26
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 20, 2021
cojenco added a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
This adds python built-in [ConnectionError](https://docs.python.org/3/library/exceptions.html#ConnectionError) to default retryable types. 
ConnectionError was recently added in the [BigQuery library](googleapis/python-bigquery#571) to allow retries. 

Fixes googleapis#426 🦕
cojenco added a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
This adds python built-in [ConnectionError](https://docs.python.org/3/library/exceptions.html#ConnectionError) to default retryable types. 
ConnectionError was recently added in the [BigQuery library](googleapis/python-bigquery#571) to allow retries. 

Fixes googleapis#426 🦕
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Python ConnectionError in list of retry
5 participants