[PECO-1259] Implement retry behavior for CloudFetch #211
Conversation
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| // 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.
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
| // 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.
|
|
||
| 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.
| @@ -265,7 +280,7 @@ describe('BaseCommand', () => { | |||
| }; | |||
|
|
|||
| 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?
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)
PECO-1259