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

Regression in max_retries usage on GetSerialPortOutput() #1243

Closed
same-id opened this issue Mar 8, 2021 · 1 comment · Fixed by #1244
Closed

Regression in max_retries usage on GetSerialPortOutput() #1243

same-id opened this issue Mar 8, 2021 · 1 comment · Fixed by #1244
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. type: docs Improvement to the documentation for an API.

Comments

@same-id
Copy link

same-id commented Mar 8, 2021

After v2.0.2, max_retries is ignored in GetSerialPortOutput().
This previously worked on v1.12.8.

All our tests are failing since they used this feature of the python library.

I traced the problem to this commit:

#1116 and #1117

c6912836 - fix: handle error on service not enabled (#1117)

1117 changed the default reason to be error.status which is PERMISSION_DENIAD instead using error.errors[0].reason which is rateLimitExceeded and therefore later ignored in:

# Only retry on rate limit related failures.
if reason in ("userRateLimitExceeded", "rateLimitExceeded"):
    return True

Example for such json:

{'error': {'code': 403,
           'errors': [{'domain': 'usageLimits',
                       'message': 'Quota exceeded for quota group '
                                  "'GetSerialPortOutputGroup' and limit "
                                  "'Instance GetSerialPortOutput requests per "
                                  "user per 100 seconds' of service "
                                  "'compute.googleapis.com' for consumer "
                                  "'project_number:REDACTED'.",
                       'reason': 'rateLimitExceeded'}],
           'message': 'Quota exceeded for quota group '
                      "'GetSerialPortOutputGroup' and limit 'Instance "
                      "GetSerialPortOutput requests per user per 100 seconds' "
                      "of service 'compute.googleapis.com' for consumer "
                      "'project_number:REDACTED'.",
           'status': 'PERMISSION_DENIED'}}

I think that correct fix for #1117 should not have changed the order of lookup - first check for error.errors[0].reason and if it doesn't exist fallback to error.status

Sam

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Mar 9, 2021
@parthea parthea self-assigned this Mar 9, 2021
@parthea parthea added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Mar 9, 2021
@parthea parthea added the type: docs Improvement to the documentation for an API. label Mar 9, 2021
@parthea
Copy link
Contributor

parthea commented Mar 9, 2021

I'm sorry that we introduced a regression! I have a potential fix in #1244. I still need to add unit tests and update the documentation.

gcf-merge-on-green bot pushed a commit that referenced this issue Mar 15, 2021
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/google-api-python-client/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [x] Ensure the tests and linter pass
- [x] Code coverage does not decrease (if any source code was changed)
- [x] Appropriate docs were updated (if necessary)

Fixes #1243 🦕
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. type: docs Improvement to the documentation for an API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants