Retry Databricks query for 503 retry after without Retry-After header
#14392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
findepi
reviewed
Sep 30, 2022
| // Databricks JDBC driver retries operations that receive HTTP 503 responses | ||
| // if the server response is returned with Retry-After headers by default. | ||
| // Following policy retries only once when received 'HTTP retry after response' without Retry-After headers. | ||
|
|
There was a problem hiding this comment.
remove this line so that the comment is close to the entity it documents
| .handleIf(throwable -> throwable.getMessage().contains("HTTP Response code: 502")) | ||
| .withBackoff(1, 10, ChronoUnit.SECONDS) | ||
| .withMaxRetries(60) | ||
| .onRetry(event -> log.warn(event.getLastFailure(), "Query failed on attempt %d, will retry.", event.getAttemptCount())); | ||
|
|
||
| // Databricks JDBC driver retries operations that receive HTTP 503 responses | ||
| // if the server response is returned with Retry-After headers by default. | ||
| // Following policy retries only once when received 'HTTP retry after response' without Retry-After headers. |
There was a problem hiding this comment.
So we want to retry "even more" than the driver does by itself, right?
I.e. we retry on top of driver's retries?
- let's make it explicit in the comment
- is there a way to configure the driver to retry for a longer time than it does by default?
There was a problem hiding this comment.
JDBC driver wouldn't retry when the response doesn't have Retry-After headers. Added retry policy covers the situation.
We can increase TemporarilyUnavailableRetryTimeout (900s by default) for failure having Retry-After headers. However, increasing the value won't resolve the recent flaky issue.
ebyhr
force-pushed
the
ebi/delta-databricks-retry
branch
from
52a6757 to
db06e4c
Compare
findinpath
reviewed
Sep 30, 2022
| // Databricks JDBC driver retries operations that receive HTTP 503 responses | ||
| // if the server response is returned WITH Retry-After headers by default. | ||
| // Following policy retries only once when received 'HTTP retry after response' WITHOUT Retry-After headers. | ||
| RetryPolicy<QueryResult> databricks503RetryPolicy = new RetryPolicy<QueryResult>() |
There was a problem hiding this comment.
Did you happen to spot this issue several times in the builds?
There was a problem hiding this comment.
findepi
approved these changes
Sep 30, 2022
|
It seems retrying is not safe in case of write operations. The actual row count is double when retried |
|
I will look for another solution as retrying for all 503 causes different issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #14391
Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: