Conversation
SantiagoPittella
added
the
no changelog
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Looks good, barring the notify change
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM, left some minor comments/questions
| _ = tokio::time::sleep(self.config.idle_timeout) => { | ||
| tracing::info!( | ||
| %account_id, | ||
| "Account actor deactivated while waiting for account commit", | ||
| ); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
Mostly asking to fully understand this change: Previously, the cancel token flow would wait indefinitely, right? If so, what is the motivation for it? I'm wondering if there is any new scenario where an actor could exit and later misses notifications
There was a problem hiding this comment.
Previously, the cancel token flow would wait indefinitely, right?
Yes
what is the motivation for it? I'm wondering if there is any new scenario where an actor could exit and later misses notifications
Notifications can't be missed, the has_pending_notification handshake in the coordinator catches this
| if self.actor_registry.contains_key(&account_id) { | ||
| tracing::error!( | ||
| account_id = %account_id, | ||
| "Account actor already exists" | ||
| ); | ||
| handle.cancel_token.cancel(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Should the existing actor in the registry be notified here?
There was a problem hiding this comment.
Hmm I'm not sure about this. why?
Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com>
3 participants
closes #1889