support retryable exceptions during query execution #368
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
dbt/adapters/sql/connections.py
Outdated
| retryable_exceptions: Iterable[Type[Exception]] = [], | ||
| retry_limit: int = 1, | ||
| retry_timeout: Union[Callable[[int], SleepTime], SleepTime] = 1, | ||
| _attempts: int = 0, |
There was a problem hiding this comment.
This feels like an early abstraction. Can we start with a timeout that's just an integer and go from there?
| _attempts: int = 0, | ||
| ) -> None: | ||
| timeout = retry_timeout(_attempts) if callable(retry_timeout) else retry_timeout | ||
| if timeout < 0: |
There was a problem hiding this comment.
This feels like java style programming, aka "look before you leap". If we remove the callable variant of the timeout, can we omit this as well? In that scenario, I think it's either our default, or the user setting, both of which can be reasonably assumed to be non-negative.
There was a problem hiding this comment.
What does removing the callable variant of the timeout mean here? We're passing in a function that the downstream user implements and I personally jive with that
| try: | ||
| execute_fn(sql, bindings) | ||
|
|
||
| except tuple(retryable_exceptions) as e: |
There was a problem hiding this comment.
This also feels like early abstraction. Can we update the type to be Tuple from start since the user of this is an adapter maintainer and not an end user?
| return self._retryable_cursor_execute( | ||
| execute_fn=execute_fn, | ||
| sql=sql, | ||
| retry_limit=retry_limit - 1, |
There was a problem hiding this comment.
We already decremented the retry limit on line 234. This does it twice.
| retry_limit=retry_limit - 1, | ||
| retry_timeout=retry_timeout, | ||
| retryable_exceptions=retryable_exceptions, | ||
| _attempts=_attempts + 1, |
There was a problem hiding this comment.
I see why we would pass in _attempts now, but this could be a separate concept, similar to the way BQ manages retries. I still think it's an early abstraction and we could update this with a more intelligent method in a future update.
resolves #
docs dbt-labs/docs.getdbt.com/#
Problem
Solution
Checklist