Skip to content

event: add scaled timer#11806

Closed
akonradi wants to merge 2 commits into
envoyproxy:masterfrom
akonradi:scaled-timer
Closed

event: add scaled timer#11806
akonradi wants to merge 2 commits into
envoyproxy:masterfrom
akonradi:scaled-timer

Conversation

@akonradi
Copy link
Copy Markdown
Contributor

Commit Message:
Add a ScaledTimer interface and impl that can be used to produce timers with min
and max timeout values. The choice of the actual duration is used is made by the
producer, independent of the user of the timer object.

Additional Description: this code provides a direct libevent-based alternative to #11773. The code isn't used anywhere, hence the low risk level.

Risk Level: low
Testing: built code
Docs Changes: none
Release Notes: none
#11427

/cc @htuch, @antoniovicente

Add a ScaledTimer interface that can be used to produce timers with min
and max timeout values. The choice of which is used is made by the
producer, independent of the user of the timer object.

Signed-off-by: Alex Konradi <akonradi@google.com>
@antoniovicente
Copy link
Copy Markdown
Contributor

/assign @antoniovicente

No need to use the actual TimerImpl in ScaledTimerImpl since it can get
by with only on the public interface methods.

Signed-off-by: Alex Konradi <akonradi@google.com>
* @param object supplies an optional scope for the duration of the alarm.
*/
virtual void enableHRTimer(const std::chrono::microseconds& min_us,
const std::chrono::microseconds& max_us,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove enableHRTimer. We don't really need it and having it adds significant complexity to the implementation; consider need for templates and visitors to perform updates.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. We should keep it in the base timer interface, since Nighthawk needs it (or discuss more with @oschaaf), but probably not needed any place you want scaled timers.

Comment thread source/common/event/timer_impl.h Outdated
* timer, only producers.
* @param scale_factor The scale factor, which will be clipped to the range [0, 1].
*/
void setScaleFactor(double scale_factor);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you plan to collect all timer instances so you can apply the scale factor to them? Note that applying the scale factor to all the instances by doing a linear scan would likely prove prohibitively expensive.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me structurally based on our last chat.

* @param object supplies an optional scope for the duration of the alarm.
*/
virtual void enableHRTimer(const std::chrono::microseconds& min_us,
const std::chrono::microseconds& max_us,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. We should keep it in the base timer interface, since Nighthawk needs it (or discuss more with @oschaaf), but probably not needed any place you want scaled timers.

@stale
Copy link
Copy Markdown

stale Bot commented Jul 7, 2020

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 stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 7, 2020
@stale
Copy link
Copy Markdown

stale Bot commented Jul 19, 2020

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!

@stale stale Bot closed this Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants