Skip to content
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

Fix running state behavior with delete #41

Closed
wants to merge 1 commit into from

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented Jul 23, 2024

When a dispatch is deleted, the running state change notification telling the actor that it is no longer running will only be sent if there are no other dispatches of the same type running.

Signed-off-by: Mathias L. Baumann mathias.baumann@frequenz.com

@Marenz Marenz requested a review from a team as a code owner July 23, 2024 13:30
@github-actions github-actions bot added part:docs Affects the documentation part:actor Affects the dispatching actor labels Jul 23, 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.

So the previous behaviour was that a stop event was sent even when there were other dispatches running for this type, so the dispatch would have stopped when it shouldn't, right?

@@ -65,6 +65,7 @@ def __init__(

self._client = client
self._dispatches: dict[int, Dispatch] = {}
self._dispatch_type_ids_map: dict[str, list[int]] = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add documentation about what this is for, as it is not trivial to figure it out.

@Marenz
Copy link
Contributor Author

Marenz commented Jul 23, 2024

So the previous behaviour was that a stop event was sent even when there were other dispatches running for this type, so the dispatch would have stopped when it shouldn't, right?

correct

When a dispatch is deleted, the running state change notification telling the actor that it is no longer running will only be sent if there are no other dispatches of the same type running.

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
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.

I think we should not apply this, as if we need to be able to handle overlapping dispatches that have different parameters, we don't get notified when one of these dispatches should stop.

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:docs Affects the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants