Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Dec 3, 2024

No description provided.

@Marenz Marenz requested a review from a team as a code owner December 3, 2024 18:26
@Marenz
Copy link
Contributor Author

Marenz commented Dec 3, 2024

Maybe this? @llucax @ela-kotulska-frequenz

@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:actor Affects the dispatching actor part:dispatcher Affects the high-level dispatcher interface labels Dec 3, 2024
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is still about recovering for a crashed actor, I would just re-create the receiver by calling running_status_change.new_receiver in the _run() instead of the constructor. This should also fix the issue, right?

Comment on lines +219 to +220
Warning: Usually you don't need to call this method. It is only useful
when you need to recover the current running state of your actor.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will highlight the warning properly:

Suggested change
Warning: Usually you don't need to call this method. It is only useful
when you need to recover the current running state of your actor.
Warning:
Usually you don't need to call this method. It is only useful
when you need to recover the current running state of your actor.

@Marenz
Copy link
Contributor Author

Marenz commented Dec 4, 2024

This should also fix the issue, right?

No, the dispatches are fetched on Dispatch.start(), not on new_receiver(). You don't get a new "current running status" if you create a new receiver

@ela-kotulska-frequenz
Copy link
Contributor

Shouldn't Dispatcher communicate with an actor by channels?

@llucax
Copy link
Contributor

llucax commented Dec 6, 2024

No, the dispatches are fetched on Dispatch.start(), not on new_receiver(). You don't get a new "current running status" if you create a new receiver

I don't understand this. So some actor at some point needs to subscribe to dispatches, they create a new receiver for status changes and start acting on it. Does this mean that with the current design each actor needs to instantiate its own dispatcher to get the proper running states because otherwise only the first actor that gets a receiver gets them?

Otherwise maybe you can explain more precisely what's the exact issue we want to solve here?

Shouldn't Dispatcher communicate with an actor by channels?

Yes, as I said in the other PR I'm OK to leave it as it is for now as a hack, as I guess it will be a while until we support inter-process communication in channels, and we'll probably need to make some adjustments everywhere, but I would at least document that this is a hack and in the future we need to use a channel to send commands to the actor.

@Marenz
Copy link
Contributor Author

Marenz commented Dec 6, 2024

Usually you set up all your receivers and channels before you even call Dispatcher.start().
Once you call start() we fetch all disptaches and inform through the channel (and DispatchManagingActor, at least optionally) all receivers about the current runtime states (whether they should be running right now).

If for any reason the receiving actor (usually that's the DispatchManagingActor, but API users might need more nuanced control for their scenarios) forgets their state and whether they should be running, that method can give them that state again.

@Marenz Marenz closed this Dec 19, 2024
@Marenz Marenz deleted the resend branch December 19, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:actor Affects the dispatching actor part:dispatcher Affects the high-level dispatcher interface part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants