Redesign actor service #103
Conversation
Marenz
commented
Jan 27, 2025
•
Marenz
commented
- Re-create actors on dispatch updates
- Rename DispatchManagingService to DispatchActorsService
- Update release notes
github-actions
bot
added
part:docs
|
|
Marenz
force-pushed
the
init-dispatch
branch
2 times, most recently
from
926cdb4 to
8350f20
Compare
|
Oh, damn, I did the review in #104 :-/ Can you maybe have a look at that and reply/continue the discussion here (by providing a link to the message you are replying to in the other PR so we don't lose context)? |
|
review? :) |
|
Mmm, I did a review of the resulting diff, so if there are no comments affecting this PR, I guess it is OK, but well, I will do a review in this PR then to double check. |
|
Correction: None of your comments relates to any commit here |
There was a problem hiding this comment.
In the commit "Rename DispatchManagingService to DispatchActorsService" it should be DispatchManagingActor, right?
I would also call it something like ActorDispatcher, we usually don't add the suffix Service to stuff inheriting from BackgroundService, but I'm also fine with the current name too.
Other comments are also minor and mostly stylistic, so approving.
| return MyActor.new_with_dispatch(initial_dispatch, dispatch_receiver) | ||
|
|
||
| managing_actor = DispatchManagingActor( | ||
| actor_factory=actor_factory, |
There was a problem hiding this comment.
In this particular case you can just use:
| actor_factory=actor_factory, | |
| actor_factory=MyActor.new_with_dispatch, |
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class DispatchUpdate: |
There was a problem hiding this comment.
As I said in the other PR, I would probably call this DispatchInfo (or maybe DispatchDetails?), as it will also represent the initial dispatch information, not just an "update". initial_dispatch: DispatchUpdate sounds a bit weird to me. new_with_dispatch(initial_details: DispatchDetails, ...) or new_with_dispatch(initial_info: DispatchInfo, ...) sounds better to me.
There was a problem hiding this comment.
So, DispatchDetail is kind of the opposite as it is an compressed / extracted dispatch.
DispatchInfo .. seems alright, I kind of don't like it, but I can't find any argument against it 😆
| def new_receiver(self) -> Receiver[DispatchUpdate]: | ||
| """Create a new receiver for dispatch updates. | ||
|
|
||
| Returns: | ||
| A new receiver for dispatch updates. | ||
| """ | ||
| return self._updates_channel.new_receiver() |
There was a problem hiding this comment.
Do you really need to expose this? I wouldn't add a public method for this if it is basically only used internally.
This commit modifies the `DispatchManagingActor` to re-create the actor when a new dispatch is received and the actor should start running instead of just start/stop the actor. To create the actor a factory function is used, which passes the initial dispatch information to the actor, so it can be properly initialized instead of having the initialization in 2 steps, the creation and the receiving of the dispatch update. 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]>
|
Addressed all comments, merging... |
