Add TimerTask#interval_type option to configure interval calculation
#997
Conversation
| # @!attribute [rw] interval_type | ||
| # @return [Symbol] method to calculate the interval between executions, can be either | ||
| # :fixed_rate or :fixed_delay; default to :fixed_delay. | ||
| def interval_type=(value) |
There was a problem hiding this comment.
Consider removing this as discussed.
There was a problem hiding this comment.
Agreed, it should be only set when building a new TimerTask and not changed after
| FIXED_RATE = :fixed_rate | ||
|
|
||
| # Default `:interval_type` | ||
| INTERVAL_TYPE = FIXED_DELAY |
There was a problem hiding this comment.
| INTERVAL_TYPE = FIXED_DELAY | |
| DEFAULT_INTERVAL_TYPE = FIXED_DELAY |
| # Default `:timeout_interval` in seconds. | ||
| TIMEOUT_INTERVAL = 30 | ||
| # Maintain the interval between the end of one execution and the start of the next execution. | ||
| FIXED_DELAY = :fixed_delay |
There was a problem hiding this comment.
As discussed, we see a third mode, which could be fixed_rate_drop which drops subsequent executions if the current execution violates the maximum time slice given the rate specified.
There was a problem hiding this comment.
Yeah that makes sense to me.
| @@ -74,6 +85,12 @@ module Concurrent | |||
| # | |||
| # #=> 'Boom!' | |||
| # | |||
| # @example Configuring `:interval_type` with either :fixed_delay or :fixed_rate, default is :fixed_delay | |||
| # task = Concurrent::TimerTask.new(execution_interval: 5, interval_type: :fixed_rate) do | |||
There was a problem hiding this comment.
I personally consider the interval_type to be a static configuration of the behaviour of the application rather than something decided at run-time. Therefore, I wonder if this would be better implemented using sub-classes which implement the specific behaviour required, e.g. FixedRateTimerTask.
There was a problem hiding this comment.
Subclasses seem rather heavy for this, and I am concerned that for compatibility we would need to keep Concurrent::TimerTask so it would be both a leaf class and an abstract class, that sounds very confusing.
So I think a constructor argument like here makes most sense and is consistent with what is already done in concurrent-ruby APIs.
There was a problem hiding this comment.
Why would it be an abstract class?
There was a problem hiding this comment.
Why would it be an abstract class?
If you make subclasses the superclass should be abstract. Otherwise I think it's pretty confusing.
|
@bensheldon if you have time, I suggest we make a 2nd PR which uses a derived class, so we can compare the level of complexity. |
|
@ioquatix no prob! I'll try to get to that next week. |
bensheldon
force-pushed
the
interval_type
branch
from
588de9a
to
c64b30d
Compare
…ulation Can be either `:fixed_delay` or `:fixed_rate`, default to `:fixed_delay`
bensheldon
force-pushed
the
interval_type
branch
from
c64b30d
to
41a35e7
Compare
|
This looks great, merging, thank you! |
Can be either
:fixed_delayor:fixed_rate, default to:fixed_delay.Closes #678
Notes:
interval_type