event: create timer from ScaledRangeTimerManager #13524

akonradi · 2020-10-12T21:19:19Z

Commit Message: Create TimerPtr from ScaledRangeTimerManager
Additional Description:
Split ScaledRangeTimerManager out as an interface, and add a new method to return
an implementation of the Timer interface directly, in addition to the method that returns
a RangeTimerImpl instance.

Risk Level: low
Testing: added unit tests
Docs Changes: n/a
Release Notes: n/a

This will allow mocking for users of the timer manager. Signed-off-by: Alex Konradi <akonradi@google.com>

Add a method to ScaledRangeTimerManager to return a TimerPtr that wraps a RangeTimerImpl. While the wrapper could be implemented externally by wrapping a RangeTimerPtr, wrapping the impl class is more efficient since it requires less indirection and fewer heap allocations. Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente

Great improvement to the scaled timer internal APIs. Thanks for splitting this off into a separate change and adding appropriate tests.

include/envoy/event/scaled_range_timer_manager.h

source/common/event/scaled_range_timer_manager_impl.cc

test/common/event/scaled_range_timer_manager_impl_test.cc

antoniovicente · 2020-10-13T02:34:24Z

test/common/event/scaled_range_timer_manager_impl_test.cc

+
+  // With the default scale factor of 1, the timer should fire at its maximum timeout.
+  MockFunction<TimerCb> callback;
+  auto timer = manager.createTimer(ScaleFactor(0.5), callback.AsStdFunction());


What happens if ScaleFactor is set to 100?

It's the same as providing a min time to a range timer that's larger than max. The max will always be honored.

My read of RangeTimerImpl::enableTimer is that the min is honored, not the max.

Sorry, what I meant was that the lesser of {min, max} will always be honored. So if min > max, then max is honored.

...and I see that the code was buggy. Added some tests and fixed it.

test/server/overload_manager_impl_test.cc

include/envoy/event/scaled_range_timer_manager.h

antoniovicente · 2020-10-13T02:38:53Z

test/common/event/scaled_range_timer_manager_impl_test.cc

+  // With the default scale factor of 1, the timer should fire at its maximum timeout.
+  MockFunction<TimerCb> callback;
+  auto timer =
+      manager.createTimer(AbsoluteValue(std::chrono::seconds(3)), callback.AsStdFunction());


Remind me, what happens when the max is smaller than an absolute min? Which of the two values wins? It seems to me that the max should win since it's the one that is more explicitly configured.

The max is always honored, even if the min is larger than the max.

My read of RangeTimerImpl::enableTimer is that the min is honored, not the max.

Signed-off-by: Alex Konradi <akonradi@google.com>

Explicitly provide the template parameters to absl::visit since using deduction causes Windows builds to fail. It appears that there is an internal instantiation of std::variant_size, which doesn't work with subclasses of absl::variant. Signed-off-by: Alex Konradi <akonradi@google.com>

include/envoy/event/scaled_range_timer_manager.h

Signed-off-by: Alex Konradi <akonradi@google.com>

…rface Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

asraa · 2020-10-20T14:34:02Z

@mattklein123 Do you have time for a non-googler/senior maintainer pass? Or pass back, thank you!

mattklein123

Thanks LGTM with a few small comments. Thank you!

/wait

mattklein123 · 2020-10-20T18:06:36Z

include/envoy/event/scaled_range_timer_manager.h

+ * Describes a minimum timer value that is equal to a scale factor applied to the maximum.
+ */
+struct ScaledMinimum {
+  explicit ScaledMinimum(double scale_factor) : scale_factor_(scale_factor) {}


I think this is the wrapper I was suggesting in the previous PR? Do we assume that this is between 0.0 and 1.0? Can we document that and ASSERT that?

As mentioned below, I'm going to leave this for a refactoring PR.

mattklein123 · 2020-10-20T18:09:18Z

include/envoy/event/scaled_range_timer_manager.h

+   * controls the minimum. Passing a value of ScaleFactor(x) sets the min to (x * max) when the
+   * timer is enabled, while AbsoluteValue(y) sets the min to the duration y.
+   */
+  virtual TimerPtr createTimer(ScaledTimerMinimum minimum, TimerCb callback) PURE;


Can RangeTimer just derive from Timer? It seems like we could avoid by allowing a RangeTimer to also be stored as a normal timer if needed?

I think the RangeTimer interface is destined for the bit bucket; we don't need it for any consumer code now that we have a createTimer method and it's not used anywhere. I think we should just remove it. Thoughts on doing that in this PR or another one?

Just do it!

Done, PTAL.

mattklein123 · 2020-10-20T18:10:25Z

include/envoy/event/scaled_range_timer_manager.h

+   * duration, a factor of 0.5 causes firing halfway between min and max, and a factor of 1 causes
+   * firing at max.
+   */
+  virtual void setScaleFactor(double scale_factor) PURE;


Should this take a ScaledMinimum (or similar) which can better ASSERT that it's between 0.0 and 1.0 and then clamp if needed?

Between here and the OverloadAction class, I think it would be a good idea to have a single simple wrapper class that enforces 0.0 and 1.0 bounds. I'm going to do that in a future PR.

Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123

Thanks!

mattklein123 · 2020-10-20T23:13:18Z

I think this might be hung due to an AZP outage. Can you try merging main?

/wait

…rface Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi added 2 commits October 12, 2020 15:49

Add interface for ScaledRangeTimerManager

f95bd82

This will allow mocking for users of the timer manager. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi mentioned this pull request Oct 12, 2020

overload: scale selected timers in response to load #13475

Merged

antoniovicente self-assigned this Oct 12, 2020

antoniovicente reviewed Oct 13, 2020

View reviewed changes

akonradi added 2 commits October 13, 2020 14:17

Address review feedback

324198d

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Oct 14, 2020

View reviewed changes

include/envoy/event/scaled_range_timer_manager.h Show resolved Hide resolved

include/envoy/event/scaled_range_timer_manager.h Show resolved Hide resolved

include/envoy/event/scaled_range_timer_manager.h Outdated Show resolved Hide resolved

akonradi added 5 commits October 14, 2020 17:26

Add comments

dc82f73

Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into scaled-timer-inte…

b3f1c99

…rface Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into scaled-timer-inte…

794e9e1

…rface Signed-off-by: Alex Konradi <akonradi@google.com>

Fix scaled timer min > max behavior

5808eed

Signed-off-by: Alex Konradi <akonradi@google.com>

Fix formatting

e29329b

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente previously approved these changes Oct 19, 2020

View reviewed changes

asraa assigned mattklein123 Oct 20, 2020

mattklein123 requested changes Oct 20, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 20, 2020

Remove RangeTimer interface

4b00ebe

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via 4b00ebe October 20, 2020 20:30

repokitteh-read-only bot removed the waiting label Oct 20, 2020

mattklein123 approved these changes Oct 20, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 20, 2020

Merge remote-tracking branch 'upstream/master' into scaled-timer-inte…

97e06c3

…rface Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only bot removed the waiting label Oct 21, 2020

mattklein123 merged commit 7e799c5 into envoyproxy:master Oct 21, 2020

akonradi deleted the scaled-timer-interface branch October 21, 2020 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

event: create timer from ScaledRangeTimerManager #13524

event: create timer from ScaledRangeTimerManager #13524

akonradi commented Oct 12, 2020

antoniovicente left a comment

antoniovicente Oct 13, 2020

akonradi Oct 13, 2020

antoniovicente Oct 16, 2020

akonradi Oct 19, 2020

akonradi Oct 19, 2020

antoniovicente Oct 13, 2020

akonradi Oct 13, 2020

antoniovicente Oct 16, 2020

asraa commented Oct 20, 2020

mattklein123 left a comment

mattklein123 Oct 20, 2020

akonradi Oct 20, 2020

mattklein123 Oct 20, 2020

akonradi Oct 20, 2020

mattklein123 Oct 20, 2020

akonradi Oct 20, 2020

mattklein123 Oct 20, 2020

akonradi Oct 20, 2020

mattklein123 left a comment

mattklein123 commented Oct 20, 2020

event: create timer from ScaledRangeTimerManager #13524

event: create timer from ScaledRangeTimerManager #13524

Conversation

akonradi commented Oct 12, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Oct 20, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented Oct 20, 2020