event: add a range timer interface#12106
Conversation
Add an interface for timers that can be enabled for some timeout within a range. The actual choice of when the timer should be triggered is not specified by the interface. Possible implementations of the interface include - timers that are triggered when a thread is not busy - timers that are triggered early/late in response to load Signed-off-by: Alex Konradi <akonradi@google.com>
|
I'm not super-attached to the name "RangeTimer" but it seemed better than everything else I came up with. "ScaledTimer" implies an implementation that the interface doesn't support, and "MinMaxTimer" seemed too verbose. |
akonradi
force-pushed
the
range-timer-interface
branch
from
ec9b68a to
d2ffc51
Compare
|
API seems fine. 'RangeTimer' is a fine name, a better name may exist but I don't have one to suggest. I assume that instances of this interface would be created via Event::Dispatcher? |
|
|
||
| /** | ||
| * An abstract event timer that can be scheduled for a timeout within a range. The actual timeout | ||
| * used is left up to individual implementations. |
There was a problem hiding this comment.
Not sure what you mean by "The actual timeout used is left up to individual implementations."
I would expect that there will be a single real implementation of this interface in Envoy, and a mock implementation.
|
The dispatcher could produce naive implementations that just trigger on the min or max, but that's not my target use case. I want to use these range timers to allow the Overload Manager to produce timers that it can trigger early if necessary. I'm working on an intermediate class similar to the one in #11773 to encapsulate the management logic. |
RangeTimers created via the dispatcher interface could get feedback from overload manager. |
|
The Overload Manager already depends on the main thread dispatcher (and the TLS for worker threads), and it seems like some kind of layering violation for the dispatcher to depend on it as well. Since RangeTimer objects can be implemented using regular Timers, I'd rather build on top of the dispatcher instead of into it. |
Timers do need to be associated with the dispatcher of the thread that owns the relevant connection/request. Overload manager being a single process wide entity makes it unsuitable for timer creation. At the very least the overload manager range timer creation method would need to accept a reference to the current thread dispatcher. |
|
My plan is to use the ThreadLocalOverloadState as the owner for the timers. It can be constructed with a reference to the dispatcher and use that to create timers in the worker thread. |
Signed-off-by: Alex Konradi <akonradi@google.com>
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
My plan is to use the ThreadLocalOverloadState as the owner for the timers. It can be constructed with a reference to the dispatcher and use that to create timers in the worker thread.
You may want to ask others for opinions. My preference continues to be for Event::Dispatcher being the sole source of objects scheduled on the event loop.
| callback_(); | ||
| return; | ||
| } | ||
| ScopeTrackerScopeState scope(scope_, *dispatcher_); |
There was a problem hiding this comment.
You need to accept dispatcher as a constructor argument, otherwise you'll run problems with this line.
|
|
||
| MockTimer::~MockTimer() = default; | ||
|
|
||
| MockRangeTimer::MockRangeTimer() { |
There was a problem hiding this comment.
Were these changes intended to be part of this PR or some future PR?
There was a problem hiding this comment.
This one. You mentioned expecting a mock, I assumed it should be included here. I can hold off on this until later though.
|
@htuch Can you weigh in re returning RangeTimers from Event::Dispatcher? Who else should be involved in the discussion? |
|
I guess I'm of the same opinion I was earlier; ideally dispatcher has a minimal interface and provides timing primitives, basically the existing timers. Then we build up more sophisticated things like range timers using these. Given the limited scope of range timers (i.e. just some parts of the data path), it doesn't seem worth cramming in there. The rationale for this layering in my mind is that by keeping dispatcher a minimal interface, we can easily port it. Whether it's to different operating system or event managers. It's also easier to reason about, e.g. if we want to do scoped accounting across events. @mattklein123 probably also has some thoughts which may or may not agree with this perspective. |
I agree with what @htuch wrote. This feels like it could just be a utility layered on top, but it's hard to say until we see its use. Can we potentially just defer this change until there is an actual use case that we can review? |
|
Thanks @antoniovicente, @htuch, and @mattklein123 for weighing in. I've sketched out the use case I envision in #11427 (comment) |
I honestly don't know what belongs or doesn't belong in the dispatcher interface. I just ran into the Event::Dispatcher::getWatermarkFactory method which added to my confusion. That interface method does predate the introduction of OverloadManager. I trust your opinions on this matter. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Commit Message: Add RangeTimer interface
Additional Description:
Add an interface for timers that can be enabled for some timeout within a range. The actual choice of when the timer should be triggered is not specified by the interface. Possible implementations of the interface include
Risk Level: low
Testing: none (interface only)
Docs Changes: none
Release Notes: none
This supersedes #11806
cc @antoniovicente