Support multiple target actors for one type of dispatch #104
Conversation
github-actions
bot
added
part:docs
Signed-off-by: Mathias L. Baumann <[email protected]>
|
@llucax ready for review |
There was a problem hiding this comment.
Just a few comments, LGTM in general, awesome to see this finally taking form!
I think it could also use some updates to the documentation, in particular getting started type of examples.
Fix broken MergeByTypeTarge.identity function
Pending: I still need to investigate why the existing tests didn't catch
this.
Is this pending for this PR? If not maybe create an issue?
| @fixture | ||
| def generator() -> DispatchGenerator: | ||
| """Return a dispatch generator.""" | ||
| return DispatchGenerator() |
There was a problem hiding this comment.
For the future, we can register new fixtures to pytest automatically in the client by making it a pytest plugin, then client users could just magically use dispatch_generator as a fixture without creating one or importing it, like we can do with mocker and other fixtures.
Signed-off-by: Mathias L. Baumann <[email protected]>
Pending: I still need to investigate why the existing tests didn't catch this. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
github-actions
bot
added
the
part:tooling
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
|
@llucax fixed release notes and added 2 further commits with features I found important after trying to integrate this interface in FCR & AFRR |
| try: | ||
| _logger.info("Starting actor for dispatch type %r", dispatch.type) | ||
| actor = self._actor_factory( | ||
| dispatch_update, | ||
| self._updates_channel.new_receiver(limit=1, warn_on_overflow=False), | ||
| ) | ||
| self._actors[self._dispatch_identity(dispatch)] = actor | ||
|
|
||
| actor.start() | ||
|
|
||
| except Exception as e: # pylint: disable=broad-except | ||
| _logger.error( | ||
| "Failed to start actor for dispatch type %r: %s", | ||
| dispatch.type, | ||
| e, | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
Not for this PR, as it is a separate feature that can be implemented separately, but I think it might be good to do the starting in a background task and retrying forever like the edge app "dispatch subsystem" does (maybe with the new dispatcher we can even get rid of the dispatch subsystem).
Not 100% sure, as doing this is making maybe too many assumptions about what clients want, but maybe it could also be opt-in/out by passing an option like in_background: bool, retries: int | INFINITE).
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
| sent_str = ", sent a dispatch update instead of creating a new actor" | ||
| await self._updates_sender.send(dispatch_update) | ||
| _logger.warning( | ||
| _logger.info( |
There was a problem hiding this comment.
I didn't get this change, the commit message says " Don't warn for changing dispatch parameters: It's a normal action ", does this mean that _start_actor is called every time a dispatch is updated? If so, I would even make this log a debug or remove it completely, because it seems misleading, as it was never the intention to start the actor if it was just a dispatch update.
If there is a way to tell if this is called because a dispatch just started or was updated, then maybe we can log more meaningful messages.
There was a problem hiding this comment.
I mean, there is a way, but it's not available to the actor_dispatcher (without restructuring).
The idea is that the dispatch instance tells us the desired state, no matter the previous state, so from that perspectives it doesn't matter whether it was an update or a new dispatch, both cases should do both, start or update a running actor instance..
There was a problem hiding this comment.
I would make this a debug then, because it is completely normal and probably not very useful to get that info when things are running. But not hung to block this PR on.
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
developed together with https://github.com/frequenz-io/frequenz-app-edge/pull/59