Skip to content

Normalize handling 403 for rate limits on 'dataset.create()'. #2089

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.

Sign up for GitHub

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

Jump to bottom
Merged
tseaver merged 3 commits into from
Aug 11, 2016
Merged

Normalize handling 403 for rate limits on 'dataset.create()'. #2089

tseaver merged 3 commits into from
Aug 11, 2016

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Aug 11, 2016

Fixes #2075.

@tseaver tseaver added testing api: bigquery Issues related to the BigQuery API. labels Aug 11, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 11, 2016
dhermes
dhermes reviewed Aug 11, 2016
View reviewed changes
system_tests/bigquery.py

# We need to wait to stay within the rate limits.
# The alternative outcome is a 403 Forbidden response from upstream.
# See: https://cloud.google.com/bigquery/quota-policy

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Explain back-end's use of 403 for rate limiting.
7dfa1ad 
A 429 ("Too many requests") would be way more appropriate than a 403
("Forbidden"), but there is nothing we can do about it here.

Addresses:
#2089 (comment)
@dhermes
Copy link
Contributor

dhermes commented Aug 11, 2016

Re: the error predicate -- I'm reluctant to add it because it makes the test flakier with respect to the back-end, which might change the error text arbitrarily and without notice.

We could deal with backend changes if / when tests fail. You can make it a bit less brittle via:

'rate limit' in error.message.lower()

@tseaver
Copy link
Contributor Author

tseaver commented Aug 11, 2016 •
edited
Loading

In 10e4f51, I figured out that we capture the JSON error structure, which means we can use the documented rateLimitExceeded reason code, rather than sniffing at the message text.

@tseaver
Add error predicate to 'retry_403'.
10e4f51 
Exclude Forbidden exceptions which do not have 'rateLimitExceed' as the
reason for one of their errors.

Addresses:
#2089 (comment).
@dhermes
Copy link
Contributor

dhermes commented Aug 11, 2016

Nice find! LGTM

@tseaver tseaver merged commit a99f269 into googleapis:master Aug 11, 2016
@tseaver tseaver deleted the 2075-bigquery-flaky-system-tests branch August 11, 2016 21:09
@dhermes dhermes mentioned this pull request Sep 19, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement. testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tseaver @dhermes @googlebot