Improve streamer events #149
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR refines the event system used by the streaming client by renaming and adjusting the stream events for clearer semantics and improved pattern matching. Key changes include:
- Removing the "Event" suffix and renaming events (e.g. StreamStartedEvent → StreamStarted, StreamStoppedEvent → StreamRetrying) and introducing a new StreamFatalError event.
- Updating tests to reflect the new event names, renaming error helper function to make_error, and adjusting expected behaviors.
- Enhancing documentation in the release notes to reflect these updates.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/streaming/test_grpc_stream_broadcaster.py | Updated tests to use new event names and adjusted error simulation. |
| src/frequenz/client/base/streaming.py | Modified event dataclasses, removed kw_only, and updated event handling. |
| RELEASE_NOTES.md | Edited release notes to align with new event names and examples. |
Comments suppressed due to low confidence (1)
tests/streaming/test_grpc_stream_broadcaster.py:48
- [nitpick] The renaming from 'mock_error' to 'make_error' provides a clearer description of the function's purpose; however, please ensure that this naming is consistent across the tests to avoid any confusion.
def make_error() -> grpc.aio.AioRpcError:
"Example:" is a section, so it cannot have a black line after it. Also it should be properly indented to be rendered correctly. Signed-off-by: Leandro Lucarella <[email protected]>
There is no need for the extra indentation inside the code block. Signed-off-by: Leandro Lucarella <[email protected]>
The suffix doesn't add a lot of extra clarity and make names too long. Signed-off-by: Leandro Lucarella <[email protected]>
Since all attributes have different types, it is not too error prone to allow using positional arguments, and this makes the dataclass automatically provides a `__match__` dunder method that allows for less verbose match syntax. We also update examples to use the match syntax to extra the event information more easily. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
This event is sent when the stream is about to stop for good due to an error (this event is NOT sent when the stream stops due to normal stream termination). Signed-off-by: Leandro Lucarella <[email protected]>
Only the types of the events were checked by the tests, not their state, which doesn't really validate everything works properly. We couldn't do a proper full comparison because we were using mocks inside the error object, and mocks have non-deterministic representations, as they have a unique ID. To fix this we just create a full gRPC error instance. This is not enough though, we also need to use a specific instance because 2 gRPC error created the same are not equal. Finally, we also need to set the retry strategy jitter to 0 so we can make sure we have a predictable retry_time. Signed-off-by: Leandro Lucarella <[email protected]>
This is just to speed up tests, we don't really need to wait. This also uncovered a bug in the creation of the `StreamStopped` instance, which was converting `0.0` to `None`. Signed-off-by: Leandro Lucarella <[email protected]>
This also ensures the stream is really being restarted. Signed-off-by: Leandro Lucarella <[email protected]>
The new event is sent only when the stream will retry reconnecting instead of every time the stream stops, as this information is less important. When the stream stops, 2 things can happen, it is retried or it finishes. The new event covers the retry case, and the case when it finishes is covered by the receiver being closed. With the current approach, we also needed to consume from the retry strategy even if no retry was going to be attempted, so with this change we can now only consume from the retry strategy when we actually want to retry. Signed-off-by: Leandro Lucarella <[email protected]>
Marenz
approved these changes
May 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains several improvements to streamer events:
Eventsuffix from stream events (for shorter names)StreamFatalErrorevent (for errors when the stream stops for good and can't be retried)StreamStoppedwithStreamRetrying(only sent when the stream is retrying, not when it stops for good)It also includes some minor improvements and fixes:
test_messages_on_retry