Repository navigation
Stop actors when a dispatch update changes its merge identity - #318
Marenz wants to merge 2 commits into
Conversation
Sorry, something went wrong.
| object.__setattr__(self, "_src", _src) | ||
|
|
||
|
|
||
| # pylint: disable=too-many-instance-attributes |
There was a problem hiding this comment.
Not necessarily for this PR, but...
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in this PR. I consolidated the new state, so the class is back within the attribute limit and the pylint suppression is gone.
Sorry, something went wrong.
| self._dispatch_identities: dict[DispatchId, DispatchActorId] = {} | ||
| """The identity to which each running dispatch was last routed.""" | ||
| self._running_dispatches: dict[DispatchId, Dispatch] = {} | ||
| """The latest version of each running dispatch.""" | ||
| self._dispatch_filter: ( | ||
| Callable[[Mapping[DispatchId, Dispatch], Dispatch], bool] | None | ||
| ) = None | ||
| self._known_dispatches: Mapping[DispatchId, Dispatch] | None = None |
There was a problem hiding this comment.
There are a few members here that are a dict DispatchId -> Something. Maybe we could have a unified _dispatch_status: dict[DispatchId, DispatchStatus] or similar, where DispatchStatus could be something like identity: DispatchActorId, running: bool.
Maybe with actors too, something like _actors mapping DispatchActorId to something having the actor, channel, and failed dispatches.
Sorry, something went wrong.
There was a problem hiding this comment.
Done. _dispatch_status now maps each dispatch ID to one DispatchStatus with the dispatch and routed identity. _actors now maps each actor identity to one ActorStatus with either the running actor/channel or the failed dispatch. I folded this into the fix commit because these status objects exist to keep the new routing state consistent.
Sorry, something went wrong.
| self._dispatch_identities[dispatch.id] = identity | ||
| self._running_dispatches[dispatch.id] = dispatch | ||
| self._failed_dispatches.pop(identity, None) |
There was a problem hiding this comment.
If we have a class for dispatch status, this can probably be done as one operation in that class, hopefully making manipulating stuff less bug prone (if we forget to update one dict it could be bad).
Sorry, something went wrong.
There was a problem hiding this comment.
Done. A start or update now replaces one DispatchStatus, and a stop pops one. The dispatch and its identity can no longer drift between separate maps.
Sorry, something went wrong.
| running_dispatch_list = [ | ||
| existing_dispatch | ||
| for existing_dispatch in dispatches.values() | ||
| if ( | ||
| self.identity(existing_dispatch) == self.identity(dispatch) | ||
| and existing_dispatch.id != dispatch.id | ||
| ) | ||
| ] | ||
|
|
||
| other_dispatches_running = any( | ||
| running_dispatch.started_at(now) | ||
| for running_dispatch in running_dispatch_list | ||
| running_dispatch_list = _other_running_dispatches( | ||
| dispatches, | ||
| dispatch.id, | ||
| self.identity(dispatch), | ||
| self.identity, | ||
| now=now, | ||
| ) | ||
| other_dispatches_running = bool(running_dispatch_list) | ||
|
|
There was a problem hiding this comment.
Is this really necessary in MergeByType? It uses type and dry_run as identity, both immutable AFAIK. I think it is only needed in MergeByTypeTarget because it adds target, which is mutable (an update can change it).
Maybe we can keep it at the top just in case anyway, in case we support updating dry_run or type, although I think it is very unlikely because it doesn't make a lot of sense (maybe dry_run, but it could still be dangerous, and I think we made it immutable because we found issues with allowing mutations for it).
Sorry, something went wrong.
There was a problem hiding this comment.
The identity comparison is still needed, but not for an identity mutation. The receiver filters only by type, so same-type dry-run and real dispatches can both reach this filter and must not suppress each other's stop events. I kept the check, added a short comment, and preserved the existing started_at(now) timing.
Sorry, something went wrong.
There was a problem hiding this comment.
Not a biggie but it would be better to split the release notes resets from the update.
Sorry, something went wrong.
There was a problem hiding this comment.
Done. The v1.0.4 reset is now its own first commit, and the new bug-fix entry stays with the fix commit.
Sorry, something went wrong.
The v1.0.4 notes were still present after the release. Restore the empty template before adding new entries. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
Remember each dispatch's routed identity in one status record. If it changes, stop the old actor unless another running dispatch still uses it. Keep failed starts with actor state so retries cannot drift from routing state. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
This branch has not been deployed
Reviewers
At least 1 approving review is required to merge this pull request.
Updating a running dispatch could change its merge identity, start a new actor, and leave the old one running.
Remember the identity each dispatch was routed to, stop the old actor when the identity changes (unless another running dispatch still uses it), and drop stale retries.
Fixes #310