-
Notifications
You must be signed in to change notification settings - Fork 20
Add RestartActorException #1169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add RestartActorException #1169
Conversation
Actor should be able to restart `_run` method for example to update formula and start listening on new receiver. Restarting actor should not print exception. Signed-off-by: Elzbieta Kotulska <[email protected]>
5009232 to
304035c
Compare
Because this nane is more descriptive. Signed-off-by: Elzbieta Kotulska <[email protected]>
6d67fb4 to
b54d916
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand where this comes from, but I'm not sure if it is the path we want to follow in the future (#632).
That said, this is how things work now and I can see how having a convenient way to restart is helpful, but an exception is also not the only way to restart, and have some implications. Mainly, anyone, at any level in the call stack, can trigger an actor restart.
An alternative that can prevent this, would be to use a asyncio.Event and await the event or the actor to finish, and then restart it if we got the event. Then adding the Actor class a restart() method that just triggers the event.
Even when Python already uses this exception mechanism for exits (like SystemExit or GeneratorExit), I'm not sure this is the right approach, because in those cases they are BaseExceptions and nobody is supposed to catch them (and body will, unless they explicitly catch BaseException),but with RestartActorException, it shouldn't inherit from BaseException (as per Python docs), but if it doesn't, it also mean anyone using except Exception, will probably avoid restarting the actor (unwillingly), so it has its risks.
All that said, since restarting actors like this should probably change in the future, I'm not sure how important is to get this perfectly right, but unless we really need the exception, I think it would be safer to go for a restart() method with an event.
| Args: | ||
| iteration: The current iteration of the restart. | ||
| """ | ||
| async def _delay(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make it a bit more explicit?
| async def _delay(self) -> None: | |
| async def _delay_restart(self) -> None: |
|
Closed, to discuss later how we should implement it |
Actor should be able to restart
_runmethodFor example to update formula and start listening on new receiver.
Restarting actor should not print exception.