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

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 May 17, 2021
@cojenco cojenco requested a review from as a code owner May 17, 2021
@google-cla google-cla bot added the cla: yes label May 17, 2021
@cojenco cojenco marked this pull request as draft May 18, 2021
Copy link
Contributor

@tritone tritone left a comment

thanks!

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


_RETRYABLE_TYPES = (
ConnectionError,
Copy link
Contributor

@tritone tritone May 18, 2021

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

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

@tseaver tseaver May 18, 2021

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

@cojenco cojenco May 18, 2021

@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

@cojenco cojenco May 18, 2021

@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

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

@tseaver tseaver May 18, 2021

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
@cojenco cojenco requested review from tseaver and tritone May 19, 2021
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 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 and removed cla: yes labels May 19, 2021
@google-cla
Copy link

@google-cla 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 tseaver commented May 19, 2021

@googlebot I fixed it.

@google-cla
Copy link

@google-cla 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 cojenco commented May 19, 2021

@googlebot I fixed it.

@google-cla
Copy link

@google-cla 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 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 and removed cla: no labels May 19, 2021
@cojenco
Copy link
Contributor Author

@cojenco 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.

@tseaver
Copy link
Contributor

@tseaver 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
鈥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 force-pushed the connection-426 branch from 581fdce to 9d7f594 May 19, 2021
@cojenco cojenco requested a review from tseaver May 19, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 8344253 into master May 20, 2021
5 checks passed
@gcf-merge-on-green gcf-merge-on-green bot deleted the connection-426 branch May 20, 2021
cojenco added a commit to cojenco/python-storage that referenced this issue 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 issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants