Make thriftbp.NewBaseplateClientPool() behave more like the grpcbp one
#602
Conversation
nanassito
force-pushed
the
thriftbp.init
branch
7 times, most recently
from
842275c
to
242517c
Compare
nanassito
requested review from
fishy,
kylelemons and
mterwill
and removed request for
a team
clientpool/channel.go
Outdated
| i++ | ||
| } else { | ||
| lastAttemptErr = err | ||
| log.Warnf("clientpool: error creating required client (will retry): %w", err) |
There was a problem hiding this comment.
I don't know that I would add a log.Warn here, I would worry about this causing a lot of log spam if there's an issue?
There was a problem hiding this comment.
@fishy really wanted a way to make the error clear to users.
There was a problem hiding this comment.
You can limit the log spam with a rate limiter:
(it should be at whatever scope you want to collect together, so global if you want to limit the spam at a per process level)
| addrGen: func() thriftbp.AddressGenerator { | ||
| i := 0 | ||
| return func() (string, error) { | ||
| i += 1 | ||
| var err error | ||
| if i == 1 { | ||
| err = fmt.Errorf("something broke") | ||
| } | ||
| return ln.Addr().String(), err | ||
| } | ||
| }(), |
There was a problem hiding this comment.
🔕 Is there any other way for us to fake these errors other than hacking them into the AddressGenerator? This isn't really the kind of error we would expect to get since most of these just return a static string (we added it to be compatible with some old ads code).
Not a blocker, but it might be nice to have the errors be closer to errors that we would actually expect/not rely on something that is really just a backwards compatibility hack.
There was a problem hiding this comment.
I don't know, that's how every other test I found does it so I used the same approach
There was a problem hiding this comment.
I've been swamped, from a cursory review this looks fine. We might want to discuss always trying at least initialConnection times (i.e. not bailing on the first failure) as well, since I think that is an innocuous behavior change, but we can do that separately.
clientpool/channel.go
Outdated
| i++ | ||
| } else { | ||
| lastAttemptErr = err | ||
| log.Warnf("clientpool: error creating required client (will retry): %w", err) |
There was a problem hiding this comment.
You can limit the log spam with a rate limiter:
(it should be at whatever scope you want to collect together, so global if you want to limit the spam at a per process level)
nanassito
force-pushed
the
thriftbp.init
branch
from
ea19e8b
to
c0a7191
Compare
Co-authored-by: Andrew Boyle <pacejackson@users.noreply.github.com>
nanassito
force-pushed
the
thriftbp.init
branch
from
c0a7191
to
edad33a
Compare
💸 TL;DR
Changing the behavior of the pool initialization to that it retries to open connections until the required number is opened instead of failing on the first issue.
📜 Details
Prior to this PR the behavior of the thriftbp pool initialization was as followed:
initialConnectionstimes to open a connection, but bail out at the first error.requiredInitialConnectionsthen crash.initialConnectionsconnections opened.This lead to reliability issues for services as the pods sometimes end up starting with very few connections ready. These connections are opened inline with the requests but that adds extra latency and opportunity for failures.
In my opinion this also is a confusing behavior since we end up returning a lower number than configured by the user. It is explicit in the documentation but I still found more than a few people surprised by this behavior.
With this PR applied the new behavior is as followed:
requiredInitialConnectionsand retry if necessary until either success or context errorinitialConnectionsusing the same behavior as before.While not my ideal approach, this allows the user to easily increase the effectively-usable number on
requiredInitialConnectionswhile leaving theinitialConnectionbehavior unchanged.Rollout:
requiredInitialConnectionsto higher values than currently.🧪 Testing Steps / Validation
✅ Checks