fix(bigquery): wrap default ResultRetryAlgorithm to retry transient HTTP errors#13416
fix(bigquery): wrap default ResultRetryAlgorithm to retry transient HTTP errors#13416jinseopkim0 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request wraps the callable in BigQueryRetryHelper.runWithRetries with a translating callable that catches IOException and throws a BigQueryException. The feedback suggests refactoring the anonymous Callable class into a lambda expression to improve readability and conciseness.
6e90263 to
3234b7c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates BigQueryRetryHelper.java to wrap the execution of the callable in a translatingCallable. This wrapper catches any IOException thrown during the retry execution and translates it into a BigQueryException. There are no review comments provided, and the changes appear to be correct and complete.
3234b7c to
7f7a7d7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates BigQueryRetryHelper to wrap callables in a translating callable that catches HttpResponseException and rethrows it as a BigQueryException. The reviewer noted that catching only HttpResponseException is too restrictive and suggested catching IOException instead to properly handle and retry other transient network issues.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates BigQueryRetryHelper.java to wrap the execution of the callable in a translating callable. This translating callable catches HttpResponseException and wraps it in a BigQueryException before rethrowing. There are no review comments, and I have no feedback to provide.
7f7a7d7 to
f9b53b0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request wraps the default BigQuery exception handler in BigQueryRetryHelper to explicitly retry on HttpResponseException for server error status codes (500, 502, 503, and 504). A new unit test is added to verify retries on 503 errors. The feedback suggests delegating createNextAttempt to the default algorithm instead of returning null to ensure robust timing logic delegation.
| @Override | ||
| public TimedAttemptSettings createNextAttempt( | ||
| Throwable previousThrowable, V previousResponse, TimedAttemptSettings previousSettings) { | ||
| return null; // Delegate timing to TimedRetryAlgorithm | ||
| } |
There was a problem hiding this comment.
Instead of hardcoding return null; in createNextAttempt, it is safer and more robust to delegate the call to defaultAlgorithm.createNextAttempt(...). This ensures that if the default algorithm's timing logic is updated in the future (e.g., to handle specific headers or custom backoff), the wrapper will correctly preserve that behavior rather than silently discarding it.
| @Override | |
| public TimedAttemptSettings createNextAttempt( | |
| Throwable previousThrowable, V previousResponse, TimedAttemptSettings previousSettings) { | |
| return null; // Delegate timing to TimedRetryAlgorithm | |
| } | |
| @Override | |
| public TimedAttemptSettings createNextAttempt( | |
| Throwable previousThrowable, V previousResponse, TimedAttemptSettings previousSettings) { | |
| return defaultAlgorithm.createNextAttempt(previousThrowable, previousResponse, previousSettings); | |
| } |
| if (algorithm == BigQueryBaseService.DEFAULT_BIGQUERY_EXCEPTION_HANDLER) { | ||
| algorithm = wrapDefaultAlgorithm(algorithm); | ||
| } |
There was a problem hiding this comment.
Can these changes be made on the specific RPCs that we want to add it to? I'm worried that this would retry on some non-query RPCs (e.g. createJob or something) which we may not want to retry.
b/467066417, b/467066596