Interface enhancements #89
Conversation
github-actions
bot
added
part:docs
tests/test_frequenz_dispatch.py
Outdated
| # pylint: disable=protected-access | ||
| actor._restart_limit = 0 |
There was a problem hiding this comment.
Maybe we should expose these fixtures publicly in the SDK in some test package?
https://github.com/frequenz-floss/frequenz-sdk-python/blob/v1.x.x/tests/conftest.py
There was a problem hiding this comment.
Could be useful, yes
There was a problem hiding this comment.
src/frequenz/dispatch/_dispatcher.py
Outdated
| # pylint: disable=redefined-builtin | ||
| def new_lifecycle_events_receiver(self, type: str) -> Receiver[DispatchEvent]: |
There was a problem hiding this comment.
Super minor but maybe call it dispatch_type instead of just type?
src/frequenz/dispatch/_dispatcher.py
Outdated
| A new receiver for new dispatches. | ||
| """ | ||
| return self._lifecycle_events_channel | ||
| return self._actor.new_lifecycle_events_receiver(type) |
There was a problem hiding this comment.
This is not following the actor model. You should only interact with an actor via channels, not calling methods.
I'm OK with keeping this approach though, as if we go multi-node at some point, probably having one dispatch-cache per mode is desirable, so all communication with the actor instance can be local. And also it is probably not worth adding the complexity needed to keep this as an actor at this point.
I just think we need to stop calling this an actor and inherit directly from BackgroundService instead, it is confusing otherwise.
Marenz
force-pushed
the
enhancefac
branch
2 times, most recently
from
e7514f4 to
49dba68
Compare
src/frequenz/dispatch/_bg_service.py
Outdated
| class DispatchingActor(Actor): | ||
| """Dispatch actor. | ||
| # pylint: disable=too-many-instance-attributes | ||
| class DispatchBackgroundService(BackgroundService): |
There was a problem hiding this comment.
Minor: maybe call it DispatchScheduler?
Signed-off-by: Mathias L. Baumann <[email protected]>
This allows us to control what happens when a new receiver is created. In this case we want to send all relevant running state events to each new receiver. It won't hurt us that other receivers might get them as well as it would just repeat the states they already know of. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
|
Updated to lucas tiny remaining feedback, will force-merge now |
2 participants
This also fixes #87