-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http: prefetch for upstreams #14143
http: prefetch for upstreams #14143
Changes from 5 commits
377a655
a3f873e
03d25da
0de2a56
00d0506
7f58cb6
800fb46
280b655
8bc15af
963803b
a130c58
47b4b7e
f4619e3
84882f2
568ac1c
5f3ef8a
228f441
7f391a9
575636e
ff6aba8
dbb54a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,28 @@ void ConnPoolImplBase::destructAllConnections() { | |
dispatcher_.clearDeferredDeleteList(); | ||
} | ||
|
||
bool ConnPoolImplBase::shouldConnect(size_t pending_streams, size_t active_streams, | ||
uint32_t connecting_capacity, float preconnect_ratio, | ||
bool anticipate_incoming_stream) { | ||
// This is set to true any time global preconnect is being calculated. | ||
// ClusterManagerImpl::maybePreconnect is called directly before a stream is created, so the | ||
// stream must be anticipated. | ||
// | ||
// Also without this, we would never pre-establish a connection as the first | ||
// connection in a pool because pending/active streams could both be 0. | ||
int anticipated_streams = anticipate_incoming_stream ? 1 : 0; | ||
|
||
// The number of streams we want to be provisioned for is the number of | ||
// pending, active, and anticipated streams times the preconnect ratio. | ||
// The number of streams we are (theoretically) provisioned for is the | ||
// connecting stream capacity plus the number of active streams. | ||
// | ||
// If preconnect ratio is not set, it defaults to 1, and this simplifies to the | ||
// legacy value of pending_streams_.size() > connecting_stream_capacity_ | ||
return (pending_streams + active_streams + anticipated_streams) * preconnect_ratio > | ||
connecting_capacity + active_streams; | ||
} | ||
|
||
bool ConnPoolImplBase::shouldCreateNewConnection(float global_preconnect_ratio) const { | ||
// If the host is not healthy, don't make it do extra work, especially as | ||
// upstream selection logic may result in bypassing this upstream entirely. | ||
|
@@ -48,9 +70,8 @@ bool ConnPoolImplBase::shouldCreateNewConnection(float global_preconnect_ratio) | |
// preconnect limit, preconnect. | ||
// We may eventually want to track preconnect_attempts to allow more preconnecting for | ||
// heavily weighted upstreams or sticky picks. | ||
if (global_preconnect_ratio > 1.0 && | ||
((pending_streams_.size() + 1 + num_active_streams_) * global_preconnect_ratio > | ||
(connecting_stream_capacity_ + num_active_streams_))) { | ||
if (shouldConnect(pending_streams_.size(), num_active_streams_, connecting_stream_capacity_, | ||
global_preconnect_ratio, true)) { | ||
return true; | ||
} | ||
|
||
|
@@ -61,8 +82,8 @@ bool ConnPoolImplBase::shouldCreateNewConnection(float global_preconnect_ratio) | |
// | ||
// If preconnect ratio is not set, it defaults to 1, and this simplifies to the | ||
// legacy value of pending_streams_.size() > connecting_stream_capacity_ | ||
return (pending_streams_.size() + num_active_streams_) * perUpstreamPreconnectRatio() > | ||
(connecting_stream_capacity_ + num_active_streams_); | ||
return shouldConnect(pending_streams_.size(), num_active_streams_, connecting_stream_capacity_, | ||
perUpstreamPreconnectRatio()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the idea here basically to use either the global ratio or the per-upstream ratio to decide on whether to make a pre-connect for this host only? If so would it be more clear to call this once and take the max of the global ratio and the per-upstream ratio? I'm confused why we would pass true for anticipate above but not here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fundamentally there's two types of prefetching, local and global. local is done every time there's a connection change, so will be adequately prefetched. It doesn't need to anticipate as it's called after new streams are assigned. global does need to anticipate, or incoming traffic to upstream A could never prefetch a connection for B (given the streams in B would be zero, it'd zero out the function). So this function will only meaningfully call one of the two blocks. I'll make it more clear with an if-else and add some more comments about the +1. |
||
} | ||
|
||
float ConnPoolImplBase::perUpstreamPreconnectRatio() const { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,6 +122,15 @@ class ConnPoolImplBase : protected Logger::Loggable<Logger::Id::pool> { | |
return *static_cast<T*>(&context); | ||
} | ||
|
||
// Determines if prefetching is warranted based on the number of streams in | ||
// use, pending streams, anticipated capacity, and preconnect configuration. | ||
// | ||
// If anticipate_incoming_stream is true this assumes a call to newStream is | ||
// pending, which is true for global preconnect. | ||
static bool shouldConnect(size_t pending_streams, size_t active_streams, | ||
uint32_t connecting_capacity, float preconnect_ratio, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pedantically for this to make sense this should be |
||
bool anticipate_incoming_stream = false); | ||
|
||
void addDrainedCallbackImpl(Instance::DrainedCb cb); | ||
void drainConnectionsImpl(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth talking about this in the arch overview docs somewhere? Seems pretty important. Feel free to do this in a follow up if you want.