Skip to content

perf(sqlalchemy-bigquery): add DB-API retry backoff with google.api_c…#17782

Closed
ohmayr wants to merge 12 commits into
mainfrom
perf-sqlalchemy-bigquery-system-tests
Closed

perf(sqlalchemy-bigquery): add DB-API retry backoff with google.api_c…#17782
ohmayr wants to merge 12 commits into
mainfrom
perf-sqlalchemy-bigquery-system-tests

Conversation

@ohmayr

@ohmayr ohmayr commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

WIP

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces transient error retries for BigQuery query execution, converts the cleanup_extra_tables test fixture into a teardown fixture, and reduces the size of the test case in test_huge_in. Feedback on these changes highlights two critical issues: first, retrying permanent errors like NotFound and AlreadyExists in the production execution path is discouraged as it introduces unnecessary delays for non-transient failures; second, wrapping the lazy list_tables iterator with a retry decorator does not protect against errors raised during iteration, which requires wrapping the list evaluation itself (e.g., using a lambda).

Comment on lines +38 to +43
retry_errors = (
"exceeded rate limits",
"already exists",
"not found",
"cannot execute dml over a non-existent table",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Retrying permanent errors such as NotFound ("not found", "cannot execute dml over a non-existent table") and AlreadyExists ("already exists") in the production query execution path (do_execute) is highly discouraged.

These errors are typically non-transient and indicate logic or schema issues. Retrying them will not resolve the error but will instead introduce unnecessary delays (up to several seconds due to backoff) before finally failing, degrading the user experience and wasting resources.

It is recommended to only retry truly transient errors, such as rate limit exceeded errors.

    retry_errors = (
        "exceeded rate limits",
    )

# Back-end may raise 403 for a dataset not ready yet.
retry_403 = test_utils.retry.RetryErrors(exceptions.Forbidden)
tables = retry_403(bigquery_client.list_tables)(bigquery_dataset)
tables = list(retry_403(bigquery_client.list_tables)(bigquery_dataset))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Because bigquery_client.list_tables returns a lazy iterator (TableIterator), the actual API call is not made when the function is called, but rather when the iterator is consumed (inside list()).

With list(retry_403(bigquery_client.list_tables)(bigquery_dataset)), the retry_403 wrapper only wraps the creation of the iterator, not its consumption. If a Forbidden error occurs during the API call, it will be raised during iteration (inside list()) and will not be retried.

To ensure the API call is properly retried, you should wrap both the creation and the consumption of the iterator in the retry block, for example by using a lambda.

Suggested change
tables = list(retry_403(bigquery_client.list_tables)(bigquery_dataset))
tables = retry_403(lambda: list(bigquery_client.list_tables(bigquery_dataset)))()

@ohmayr
ohmayr force-pushed the perf-sqlalchemy-bigquery-system-tests branch from 9fb1d5f to 9fdd7ed Compare July 20, 2026 21:34
@ohmayr
ohmayr force-pushed the perf-sqlalchemy-bigquery-system-tests branch from 3cd47ba to dc15fd0 Compare July 21, 2026 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant