-
Couldn't load subscription status.
- Fork 6
Bug fixes after testing with edge app #169
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
Conversation
Marenz
commented
Jun 26, 2025
- Fix sdk depency after yanked release
- Explictly require newer time when comparing in fetch()
- Harden sync for delete events between stream/fetch
- Harden stream/fetch sync for CREATED and UPDATED
Signed-off-by: Mathias L. Baumann <[email protected]>
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.
Pull Request Overview
This PR addresses bug fixes related to dispatch synchronization and dependency updates after testing with an edge application. Key changes include updating the SDK dependency version, introducing logic to handle deleted dispatches, and tightening update conditions to ensure proper event processing.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frequenz/dispatch/_bg_service.py | Adds deleted dispatch tracking, refines event handling for CREATED, UPDATED, and DELETED events |
| pyproject.toml | Updates the SDK dependency version range to a newer release |
Comments suppressed due to low confidence (2)
src/frequenz/dispatch/_bg_service.py:314
- Clarify in a comment why is_more_relevant() is used in the deletion event block and how it aligns with the intended deletion logic, to aid future maintainers in understanding the decision.
if is_more_relevant(dispatch):
src/frequenz/dispatch/_bg_service.py:396
- Verify that using a strict '>' comparison for dispatch.update_time handles all intended update cases; if equal timestamps should trigger an update, consider revising the condition accordingly.
elif dispatch.update_time > old_dispatch.update_time:
src/frequenz/dispatch/_bg_service.py
Outdated
|
|
||
| def is_more_relevant( | ||
| dispatch: Dispatch, | ||
| ) -> bool: |
Copilot
AI
Jun 26, 2025
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.
[nitpick] Consider adding a docstring or inline comment explaining the purpose and usage of is_more_relevant() to improve code readability and maintainability.
| ) -> bool: | |
| ) -> bool: | |
| """ | |
| Determine if a dispatch is more relevant than an existing one. | |
| A dispatch is considered more relevant if it does not exist in the | |
| current dispatches or if its update time is more recent than the | |
| existing dispatch's update time. | |
| Args: | |
| dispatch: The dispatch to evaluate. | |
| Returns: | |
| True if the dispatch is more relevant, False otherwise. | |
| """ |
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.
All optional (although I strongly recommend avoiding not x when you really mean x is None).
Disabling auto-merge to give you the opportunity to address some of the feedback.
src/frequenz/dispatch/_bg_service.py
Outdated
|
|
||
| def is_more_relevant( | ||
| dispatch: Dispatch, | ||
| ) -> bool: | ||
| existing_dispatch = self._dispatches.get(dispatch.id) | ||
|
|
||
| return ( | ||
| not existing_dispatch | ||
| or dispatch.update_time > existing_dispatch.update_time | ||
| ) | ||
|
|
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.
Since you are calculating this for every case, why don't you just store it in a variable instead of creating a nested function? Also, is_more_relevant() seems a bit too vague, what about is_new_or_newer?
src/frequenz/dispatch/_bg_service.py
Outdated
| _logger.debug( | ||
| "Received dispatch event: %s", selected.message | ||
| ) | ||
| dispatch = Dispatch(selected.message.dispatch) |
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.
It might also add some clarity to rename dispatch to new_dispatch and use old_dispatch for the one we have stored, although is out of the scope of this PR, but mentioning it because it related.
I also strong recommend to use is None instead of not, as if in the future someone adds a __bool__ to dispatch, the meaning could change. In general is better practice to use is None than implicit conversion to bool if you want to check if something returned None, we already had several bugs because we used not x with int/float when the value was 0.
| dispatch = Dispatch(selected.message.dispatch) | |
| new_dispatch = Dispatch(selected.message.dispatch) | |
| old_dispatch = self._dispatches.get(new_dispatch.id) | |
| is_new_or_newer = ( | |
| old_dispatch is None | |
| or new_dispatch.update_time > old_dispatch.update_time | |
| ) | |
src/frequenz/dispatch/_bg_service.py
Outdated
| # Check if the dispatch already exists and | ||
| # was updated. The CREATE event is late in | ||
| # this case | ||
| if is_more_relevant(dispatch): |
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.
| if is_more_relevant(dispatch): | |
| if is_new_or_newer: |
etc.
| elif dispatch.update_time != old_dispatch.update_time: | ||
| elif dispatch.update_time > old_dispatch.update_time: |
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.
Explictly require newer time when comparing in fetch()
It would be good to specify in the commit message why this change is needed or better. Maybe it is because of the limited diff context, but it is not obvious for me.
Signed-off-by: Mathias L. Baumann <[email protected]>
We remember delete events with their timestamp, so when can check in fetch() if the dispatch we just fetched is actually already deleted before we received it. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>