Reduce HTTP retry rate from 5 to 3 #557
Conversation
I think even if we're in a wait-and-retry, a context cancellation should be able to stop that and exit immediately - there is definitely a bug there somewhere that needs fixing, separately from this PR. |
| @@ -405,7 +405,7 @@ func CreateHTTPClient(conf ServerConfig, logger *util.Logger, retry bool) *http. | |||
| client := retryablehttp.NewClient() | |||
| client.RetryWaitMin = 1 * time.Second | |||
| client.RetryWaitMax = 30 * time.Second | |||
| client.RetryMax = 4 | |||
| client.RetryMax = 2 | |||
There was a problem hiding this comment.
Can we map out the effective difference of how long we waited at most before in total, vs how long we wait now? (based on https://github.com/hashicorp/go-retryablehttp/blob/v0.7.0/client.go#L470)
Mainly I'm wondering if there is an argument that its good to wait a few minutes if our backend services are temporarily unavailable (if the max was always used, previously we waited at most 2 mins, and now we'd wait at most 1 minute -- but I think in practice those numbers would be lower, given the exponential backoff logic that's capped to that max)
There was a problem hiding this comment.
As I remember it, HTTP retry was added to work around temporary network connectivity issues, not to smooth over API outages (which should be very rare).
@seanlinsley FYI, I think I found the root cause(s) of the cancellation behavior: #563 and #565 So maybe we don't need this PR if we merge those two instead? |
5 attempts is probably too much, especially when each can take 30 seconds (or is it 120 seconds from
Timeout?). This might explain why we've seen the collector take so long to shut down after pressing Control-C.For context:
RetryMaxis the max retries after an initial failure, so the actual number of requests will be1 + RetryMax