[PECO-1259] Implement retry behavior for CloudFetch - #211
Conversation
Sorry, something went wrong.
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
This comment was marked as outdated.
susodapop
left a comment
There was a problem hiding this comment.
Left a couple comments.
Sorry, something went wrong.
|
|
||
| // Delay interval depends on current attempt - the more attempts we do | ||
| // the longer the interval will be | ||
| // TODO: Respect `Retry-After` header (PECO-729) |
There was a problem hiding this comment.
Yep, this is important. When the server responds with 429 or 503 it includes a Retry-After header that we should monitor.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, that's why we have PECO-729. This PR (and this particular code) is almost untouched, just moved to a different place so it could be used for cloudfetch. After we merge this, I'm going to make it match PySQL's behavior, as you mentioned in your other comment below
Sorry, something went wrong.
| // TODO: Respect `Retry-After` header (PECO-729) | ||
| const retryDelay = getRetryDelay(this.attempt, clientConfig); | ||
|
|
||
| const attemptsExceeded = this.attempt >= clientConfig.retryMaxAttempts; |
There was a problem hiding this comment.
Nit: I think you can bump this logic up a few lines. If we already exceeded the maximum number of attempts there's no reason to get the retry delay.
Sorry, something went wrong.
|
|
||
| return { shouldRetry: true, retryAfter: retryDelay }; | ||
|
|
||
| // TODO: Here we should handle other error types (see PECO-730) |
There was a problem hiding this comment.
For future reference, here's the retry policy used in the python connector: https://github.com/databricks/databricks-sql-python/blob/3f6834c9797503132cb0d1b9b770acc36cd22d42/src/databricks/sql/auth/retry.py#L308.
Sorry, something went wrong.
|
|
||
| const command = new CustomCommand( | ||
| new ThriftClientMock(() => { | ||
| new ThriftClientMock(context, () => { |
There was a problem hiding this comment.
Do you have an integration test that verifies when requests are retried?
Sorry, something went wrong.
There was a problem hiding this comment.
Not yet, but I'm going to add some after implementing PECO-729 (which should wrap up the whole retry feature)
Sorry, something went wrong.
susodapop
left a comment
There was a problem hiding this comment.
Approving as this is a strict refactor. Improvements to come in a subsequent PR.
Sorry, something went wrong.
PECO-1259