Add backoff package and fix Consul CPU usage#635
Conversation
Justification for jitter and growth factor: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/. Add backoff to the Consul instancer loop. Fixes go-kit#627.
|
If I may chime in with one comment about backoff package api. Overall, the
From the minor stuff I think test should not call directly unexported methods of the type being tested. |
|
Sorry I didn't see this originally, late last year was hectic time for me :( |
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
Feel like too much. What do you think of my alternative? Am I missing a key ingredient?
backoff/backoff.go
Outdated
|
|
||
| currentInterval atomic.Value | ||
| cancel <-chan struct{} | ||
| } |
There was a problem hiding this comment.
Mixing exported and non-exported fields can be OK, but then you also mix direct and constructor initialization for this type, and combined I think that's a recipe for trouble...
backoff/backoff.go
Outdated
| d := float64(current * 2) | ||
| jitter := rand.Float64() + 0.5 | ||
| return time.Duration(d * jitter) | ||
| } |
There was a problem hiding this comment.
It seems like this jitter bit is the real meat of the change. Rather than adding a new package, types, API, package docs, etc. — can you instead put this jitter calculation into the old exponential func, statelessly?
| d = time.Minute | ||
| } | ||
| return d | ||
| } |
There was a problem hiding this comment.
I'm thinking a 1-line change can be enough, something like this, feel free to make corrections...
func exponential(d time.Duration) time.Duration {
d *= 2
if d > time.Minute {
d = time.Minute
}
return d * (rand.Float64() + 0.5)
}
|
@nicot Ping? I'd love to be able to close the corresponding issue :) |
|
Thanks for the review @rjeczalik and @peterbourgon. I like your simple version a lot more @peterbourgon. There's a little bit of casting funkiness because duration is an int64 and we want to multiply it by a float, so let me know if there is a better way to do that. Do you think the Exponential function belongs in the util/conn package? Or should it be somewhere more accessible? |
|
@peterbourgon ping back at you =) |
| } | ||
|
|
||
| func exponential(d time.Duration) time.Duration { | ||
| func Exponential(d time.Duration) time.Duration { |
There was a problem hiding this comment.
Can we get some doc string here? :)
sd/consul/instancer.go
Outdated
| default: | ||
| s.cache.Update(sd.Event{Instances: instances}) | ||
| time.Sleep(d) | ||
| d = conn.Exponential(d) |
There was a problem hiding this comment.
Sorry for not catching this earlier. We want to implement an upper limit e.g. 30s, and have a successful reconnect reset the d value to its default state.
sd/consul/instancer.go
Outdated
| var ( | ||
| instances []string | ||
| err error | ||
| d time.Duration = time.Millisecond * 10 |
There was a problem hiding this comment.
d = 10 * time.Millisecond is preferable here.
|
Still need an upper limit. |
|
The upper limit is in the Exponential function -- Do you want both? |
|
Oh! Gosh. Brainfart. Thanks! |
3 participants
Justification for jitter and growth factor: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/.
Add backoff to the Consul instancer loop.
Fixes #627.