Repository navigation
Retry transient errors in BigqueryClient.queryUnflattened - #40793
mxtymoshyk wants to merge 2 commits into
Conversation
queryUnflattened polled jobs.getQueryResults with no retry, so one 503 backendError failed an integration test after its pipeline had already succeeded. Poll through getQueryResultsWithRetries, which uses the same backoff as the other BigqueryClient methods. It retries 5xx and 429 and throws other 4xx errors right away. Also delete the temporary dataset in a finally block. Before, the method skipped the delete when the query returned no rows or threw.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR improves the resilience of BigqueryClient.queryUnflattened in integration tests by adding retry logic with backoff when polling jobs.getQueryResults for transient server errors (5xx) and rate limits (429), while failing fast on other 4xx errors. Additionally, it fixes a resource leak by moving the temporary dataset deletion into a finally block.
🔍 General Feedback
- The implementation aligns well with existing retry patterns in
BigqueryClient. - Good catch on both the
Boolean.TRUE.equals(qResponse.getJobComplete())null-safety check and ensuring temporary datasets are cleaned up when queries return empty results or throw exceptions. - The new unit tests in
BigqueryClientTestcleanly cover the retry behavior, fast-fail on 4xx, and dataset cleanup.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Polling getQueryResults again can't fix a job that itself failed with a transient error such as a 503 backendError, because every poll returns the same error. Each retry now starts a new query job, which also covers a 503 from jobs.insert. The temp table uses WRITE_TRUNCATE so a retried job overwrites anything a failed job left behind. The retry check also treats 403 rateLimitExceeded as transient, applies the status-code rules to non-JSON HTTP errors, and keeps the original jobComplete check so a missing value fails instead of looping.
|
Assigning reviewers: R: @ahmedabu98 for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Fixes #40792.
Many BigQuery integration tests check their output with
BigqueryClient.queryUnflattened. It started one query job and polledjobs.getQueryResultswith no retry, so a single transient 503backendErrorfailed a test after its pipeline had already succeeded. This happened inStorageApiDataTriggeredSchemaUpdateITon #40790.The 503 there read "Error encountered during execution", which means the query job itself failed. Polling that job again returns the same error, so the retry has to start a new job.
queryUnflattenednow runs the query throughrunQueryJobWithRetries. On a transient error fromjobs.insertorjobs.getQueryResults, it starts a new job, with the same backoff the otherBigqueryClientmethods use.rateLimitExceeded(checked withApiErrorExtractor, asBigQueryServicesImpldoes) or a failure with no HTTP response. Other 4xx errors, such as an invalid query, are thrown right away.WRITE_TRUNCATE, so a retried job overwrites anything a failed job wrote.finallyblock. Before, the method skipped the delete when the query returned no rows or threw.BigqueryClientTesthas 6 new tests: a new job after a 503 from polling, a retry after a 503 fromjobs.insert, a retry after a 403rateLimitExceeded, no retry on a 400 or a non-JSON 404, and cleanup when the query returns no rows. 5 of them fail on master. The non-JSON 404 test passes on master too, because master never retries, and it guards the new retry check.