Merge & unify running state event intervals #94
Conversation
Signed-off-by: Mathias L. Baumann <[email protected]>
github-actions
bot
added
part:docs
Marenz
requested review from
llucax
and removed request for
stefan-brus-frequenz
There was a problem hiding this comment.
Interesting, I always thought merging multiple dispatches in one big period was the default behavior.
LGTM for now, but in the future I think we should really go for extensible merge strategies / policies, so we can implement merging by target too for example.
I guess to implement this externally as merge strategy/policy objects we would need to expose more of the internal state of the dispatcher and pass the dispatcher to the strategy object, or maybe just passing the list of dispatches would be enough too.
src/frequenz/dispatch/_bg_service.py
Outdated
| """A queue item for the scheduled events.""" | ||
|
|
||
| time: datetime | ||
| stop_event: bool |
There was a problem hiding this comment.
| stop_event: bool | |
| is_stop_event: bool |
Otherwise it sounds like an action? Or even maybe an event enum with START/STOP as it would be more extensible for other potential events? But probably that's overkill at this stage.
BTW, I still don't understand what this is for, it even seems to be unused!
There was a problem hiding this comment.
Given two events at the same time:
- Dispatch 12 - STOP
- Dispatch 13 - START
As the list of events is ordered based on (event time, id),
we would in this case first run dispatch 12 as the time matches and the id smaller.
What we want, however is to run always the START event if the time is equal. Because then the filters looking at the events had a chance to observe START before STOP and thus can filter out the STOP when desired.
it even seems to be unused!
We don't need to directly reference it, the ordering is done by the dataclass
There was a problem hiding this comment.
Oh, this is really sneaky! So you are relying on the order between True and False? You naughty naughty boy...
Then I would definitely clarify this in a comment and probably even make it a priority: int (or a IntEnum) to make it even more clear that this is in the item only to affect the order in the queue.
There was a problem hiding this comment.
Sneaky? I call that elegant.
Marenz
force-pushed
the
mergy
branch
2 times, most recently
from
efb289c to
6a3d2db
Compare
RELEASE_NOTES.md
Outdated
| * `Dispatcher.running_status_change` has been replaced by the method `Dispatcher.new_running_state_event_receiver(self, type: str)`. | ||
| * `Dispatcher.lifecycle_events` has been replaced by the method `Dispatcher.new_lifecycle_events_receiver(self, dispatch_type: str)`. | ||
| * `Dispatcher.running_status_change` has been replaced by the method `Dispatcher.new_running_state_event_receiver(self, dispatch_type: str, unify_running_intervals: bool)`. | ||
| * A new feature "unify running intervals" has been added to the `Dispatcher.new_running_state_event_receiver` method. Using it, you can automatically merge & unify consecutive and overlapping dispatch start/stop events of the same type. E.g. dispatch `A` starting at 10:10 and ending at 10:30 and dispatch `B` starts at 10:30 until 11:00, with the feature enabled this would in total trigger one start event, one reconfigure event at 10:30 and one stop event at 11:00. |
There was a problem hiding this comment.
Why is this in Upgrading and now New Features?
There was a problem hiding this comment.
Well, it is kind of both?
There was a problem hiding this comment.
The new option is, optional, so it is backwards compatible, that's what I meant and what I see you put in the release notes 👍
src/frequenz/dispatch/_bg_service.py
Outdated
| async def new_running_state_event_receiver(self, type: str) -> Receiver[Dispatch]: | ||
| """Create a new receiver for running state events. | ||
| async def new_running_state_event_receiver( | ||
| self, type: str, unify_running_intervals: bool = True |
There was a problem hiding this comment.
| self, type: str, unify_running_intervals: bool = True | |
| self, type: str, *, unify_running_intervals: bool = True |
This allows us to filter the `stop` events when we desire to merge consecutive events. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
2 participants
startevents are executed beforestopevents