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

[BigQuery] Retry on rateLimitExceeded response #2795

Closed
championj-foxtel opened this issue Sep 28, 2020 · 3 comments · Fixed by #2796
Closed

[BigQuery] Retry on rateLimitExceeded response #2795

championj-foxtel opened this issue Sep 28, 2020 · 3 comments · Fixed by #2796
Labels
enhancement New feature or request

Comments

@championj-foxtel
Copy link
Contributor

championj-foxtel commented Sep 28, 2020

Describe the feature

On a response of 403 rateLimitExceeded from BigQuery the tasks should retry if there are tries remaining

Additional context

rateLimitExceeded error in BigQuery returns a 403 instead of 429 https://cloud.google.com/bigquery/docs/error-messages

See similar discussion for google-cloud-python for some context: googleapis/google-cloud-python#6434

Who will this benefit?

When a temporary rate limit has been exceeded a retry here will prevent the pipeline from failling

Are you interested in contributing this feature?

I have patched this locally just checking for Forbidden response type; this should be improved to inspect error reason for 'rateLimitExceeded' or error message for 'Exceeded rate limits' or similar filter to only retry in rate limit exceeded scenario.

# plugins/bigquery/dbt/adapters/bigquery/connections.py
def _is_retryable(error):
    """Return true for errors that are unlikely to occur again if retried."""
    if isinstance(error, RETRYABLE_ERRORS):
        return True
    elif isinstance(error, google.api_core.exceptions.Forbidden) and any(e['reason'] == 'rateLimitExceeded' for e in error.errors):
        return True
    return False
@championj-foxtel championj-foxtel added enhancement New feature or request triage labels Sep 28, 2020
@drewbanin
Copy link
Contributor

wow - thanks for the heads up @championj-foxtel! I did not know that BQ returns a 403 here.... that is quite a decision.

Your local patch looks really great! If you'd be interested in opening a PR for that change, I think we'd be really happy to merge it. Let us know if that's something you'd be interested in - happy to help out if so!

@drewbanin drewbanin removed the triage label Sep 28, 2020
@championj-foxtel
Copy link
Contributor Author

Hi @drewbanin - that sounds great I will raise a PR for this one!

@jtcohen6
Copy link
Contributor

Resolved by #2796

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants