connect: use exp backoff when waiting on consul envoy bootstrap #10453
Conversation
shoenig
force-pushed
the
b-envoy-wait-longer
branch
from
0143218
to
33cf70a
Compare
| @@ -0,0 +1,142 @@ | |||
| // Package exptime provides a generalized exponential backoff retry implementation. | |||
There was a problem hiding this comment.
We implement "exponential back-off" in a few places, but none of them generic and feature-full. It'd be nice to converge on a single implementation.
|
I'm wondering if we should backport this |
There was a problem hiding this comment.
LGTM once we get the license in there. The library seems like the right one to use; they're accounting properly for the time it takes to perform the callback, which I've seen get missed a lot.
I'll leave it up to your judgement as to whether we want to classify this as a bugfix or an improvement in the changelog. If it's a bugfix, we should backport it.
|
|
||
| // A Sleeper is a useful way for calling time.Sleep | ||
| // in a mock-able way for tests. | ||
| type Sleeper func(time.Duration) |
There was a problem hiding this comment.
It looks like this alias is the only diff between oss.indeed.com/go/libtime/decay and this source file? While I understand and agree with not wanting to pull in the whole darn libtime library for this, the "derived work" being created here is an awfully minimal change. So for licensing safety, I think we're going to need to include the upstream license file in this package.
There was a problem hiding this comment.
👍 added the license
But yeah, the only reason I don't want to pull it in as a module is the transitive dependency on github.com/gojuno/minimock/v3 is several orders of magnitude larger than this one function we actually want
| // account for how long we intended to sleep | ||
| consumed += duration | ||
|
|
||
| // exponentially increase the gap |
There was a problem hiding this comment.
This is fine because we should avoid creating more of a diff here, but "Dear libtime library authors, 2x per iteration is geometric backoff, not exponential." 😀
shoenig
force-pushed
the
b-envoy-wait-longer
branch
from
33cf70a
to
b4c10a2
Compare
This PR wraps the use of the consul envoy bootstrap command in an expoenential backoff closure, configured to timeout after 60 seconds. This is an increase over the current behavior of making 3 attempts over 6 seconds. Should help with #10451
shoenig
force-pushed
the
b-envoy-wait-longer
branch
from
b4c10a2
to
6884455
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR wraps the use of the consul envoy bootstrap command in
an expoenential backoff closure, configured to timeout after 60
seconds. This is an increase over the current behavior of making
3 attempts over 6 seconds.
Should help with #10451