overload: remove createScaledTimer with OverloadTimerType #14536
Conversation
Require that callers of ThreadLocalOverloadState::createScaledTimer know their timer scaling factor instead of looking it up by well-known type. This makes timer creation more efficient since the lookup is performed at most once instead of on every call to createScaledTimer, and will make it easier to add scaled timers in other places without threading the entire ThreadLocalOverloadState there. Signed-off-by: Alex Konradi <akonradi@google.com>
|
/assign @antoniovicente |
| @@ -101,6 +101,10 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, | |||
| overload_state_.getState(Server::OverloadActionNames::get().StopAcceptingRequests)), | |||
| overload_disable_keepalive_ref_( | |||
| overload_state_.getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)), | |||
| downstream_idle_connection_scaled_timeout_(overload_manager.getConfiguredTimerMinimum( | |||
| Server::OverloadTimerType::HttpDownstreamIdleConnectionTimeout)), | |||
There was a problem hiding this comment.
The scale lookups could be made constant by using an array as a representation instead of a map. The new API seems more complex. What are your thoughts?
There was a problem hiding this comment.
It's still an extra lookup. The goal of this change is to allow implementing the TLS handshake scaled timeout without plumbing around the entire thread-local overload state. Instead we can pass around a ScaledTimerMinimum.
There was a problem hiding this comment.
Is the API more usable with the lookup? We should optimize for humans rather than micro optimizing machine cycles.
There was a problem hiding this comment.
The goals here are
- not plumb ThreadLocalOverloadState all over the place for overload: scale transport socket connect timeout #13800
- make it possible to move scaled timer creation into the Dispatcher without also having it hold on to the random map (or array) reference to look up the minimum from the type
- keep things efficient at runtime :)
There was a problem hiding this comment.
This all started with the SSL connection case. How does this change address the need to plumb this minimum to ServerConnectionImpl's constructor as done in https://github.com/envoyproxy/envoy/pull/13800/files#diff-3ee563f926e7a290ac2d07369316327fbdf327c89db4a9faef2bf382e33fc0c2 ?
Having the creation mechanism in the dispatcher take an enum clearly answers the plumbing question. It is less clear how to address the plumbing issue after this change.
There was a problem hiding this comment.
We'd still need to plumb the minimum there, but as a value object, not a reference with potential lifetime issues. I'm imagining ServerConnectionImpl::setTransportSocketConnectTimeout would call dispatcher_->createScaledTimer with the minimum value from the constructor.
There was a problem hiding this comment.
It may be worth sketching out the full change. At least my pushback on that PR was related to the large nature of the changes. Having to plumb the minimum would still require extensive changes. Please talk to @ggreenway about his opinion about this change.
There was a problem hiding this comment.
Sure, here's my thinking. Right now, the ThreadLocalOverloadState (TLOS) is used to create scaled timers, either by providing a ScaledTimerMinimum value or an OverloadTimerType enum value. If the enum is provided, the TLOS does a lookup in its table to find the mapped ScaledTimerMinimum, if any. Assuming we keep the OverloadTimerType enum, we have to have that map somewhere. Here are some options I've come up with:
- make the map available to the TLOS and use it to do the lookup and create timers (this is what we have now)
- move timer creation to the Dispatcher, but make the map available via the singleton OverloadManager and pass the ScaledTimerMinimum to a new Dispatcher::createScaledTimer method (this would require plumbing the ScaledTimerMinimum for the TLS handshaker everywhere)
- move timer creation to the Dispatcher, put the map in the Dispatcher, and pass OverloadTimerType to Dispatcher::createScaledTimer
I'm not a big fan of 3 since it feels like adding bloat to the DispatcherImpl. I do like that it doesn't require plumbing a bunch of stuff everywhere, though.
There was a problem hiding this comment.
What are your thoughts of having the ScaledTimerManager keep the mapping of enum values to minimums? The dispatcher would own the ScaledTimerManager and delegate creation of scaled timers to it.
There is still the question of how to get the map to the dispatcher constructor. I'm guessing that passing in the singleton overload manager to the dispatcher constructor would be a suitable solution. Passing in a thread local overload state to the dispatcher constructor would also work. Acquiring the map on the first overload state update would be fine too.
There was a problem hiding this comment.
That's not too terrible. We'd want to have a way of injecting/modifying the ScaledTimerManager in the Dispatcher already for testing purposes, and we could use that here as well. I'll play around with different ways of getting the map in.
|
Is this PR obsolete? If yes, please close. |
Commit Message: Remove createScaledTimer method that takes OverloadTimerType
Additional Description:
Require that callers of ThreadLocalOverloadState::createScaledTimer know
their timer scaling factor instead of looking it up by well-known type.
This makes timer creation more efficient since the lookup is performed
at most once instead of on every call to createScaledTimer, and will
make it easier to add scaled timers in other places without threading
the entire ThreadLocalOverloadState there.
Risk Level: low
Testing: ran affected tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a