Make PoolOutOfServers error retryable. #675
Conversation
When multiple goroutines access the pool concurrently, one may remove all servers from the routing table while another is trying to borrow a connection, resulting in a PoolOutOfServers error. This error should trigger a retry with a fresh routing table fetch, similar to PoolTimeout. - Add PoolOutOfServers to retryable errors in IsRetryable(). - Wrap PoolOutOfServers as ConnectivityError for consistency.
| if _, ok := err.(*errorutil.PoolOutOfServers); ok { | ||
| return true | ||
| } |
There was a problem hiding this comment.
If my search-foo didn't let me down, I don't think that IsRetryable is ever being called on an unwrapped error. Therefore, I doubt this code-path ever gets hit.
If, however, we want IsRetryable to also work for unwrapped errors, there'd be more to do here. Many errors get wrapped in ConnectivityError which makes them retryable. So all of those would probably also need a special check here.
Not a blocker though, as I doubt it'll break anything. Except for devs after us wondering why this if was put here with some intention 🙃
There was a problem hiding this comment.
Yeah, good catch, in my head, IsRetryable is a public API, therefore I thought we needed a case for the error for users with custom retry logic, but the internal retry flow in Continue()
wraps errors before checking (IsRetryable(errorutil.WrapError(lastErr))), so the direct check never gets hit there.
Happy to remove this, especially if there needs to be more work anyway to make other unwrapped errors work correctly - what's your thoughts?
There was a problem hiding this comment.
therefore I thought we needed a case for the error for users with custom retry logic
Will such user-written retry loop see the wrapped or unwarpped error? If it ever sees the unwrapped/raw error (PoolOutOfServers), then I totally agree, that this check should remain (and other errors might also need to be added here).
When multiple goroutines access the pool concurrently, one may remove all servers from the routing table while another is trying to borrow a connection, resulting in a
PoolOutOfServerserror. This error should trigger a retry with a fresh routing table fetch, similar toPoolTimeout.PoolOutOfServersto retryable errors inIsRetryable().PoolOutOfServersasConnectivityErrorfor consistency.