Client.stream(): Return a streamer instead of a receiver#179
Client.stream(): Return a streamer instead of a receiver#179Marenz wants to merge 1 commit intofrequenz-floss:v0.x.xfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates DispatchApiClient.stream() to return a GrpcStreamBroadcaster (a streamer) instead of a simple receiver, allowing callers to spawn customizable receivers via new_receiver().
Key changes:
- Refactored
stream()implementation to return a broadcaster and inlined stream-management logic. - Updated tests to call
stream.new_receiver()before receiving messages. - Documented the breaking change and example usage in
RELEASE_NOTES.md.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_client.py | Updated test to use stream.new_receiver().receive() |
| src/frequenz/client/dispatch/_client.py | Changed return type of stream(), removed _get_stream, inlined logic |
| RELEASE_NOTES.md | Added note about stream() now returning a streamer and example |
Comments suppressed due to low confidence (3)
src/frequenz/client/dispatch/_client.py:239
- The Returns section is outdated: update it to indicate this method returns a streamer (
GrpcStreamBroadcaster), not a receiver.
A receiver channel to receive the stream of dispatch events.
tests/test_client.py:313
- [nitpick] Consider renaming the variable
streamto something likestreamerto reflect that it’s a broadcaster and avoid confusion with the receiver returned bynew_receiver().
message = await stream.new_receiver().receive()
src/frequenz/client/dispatch/_client.py:213
- The signature references
GrpcStreamBroadcasterbut it isn’t imported. Addfrom frequenz.client.base.channel import GrpcStreamBroadcasterto the top of the file.
def stream(
This allows us to use all the options available in the streamer. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
There was a problem hiding this comment.
I'm not convinced about this, GrpcStreamBroadcaster is a "low-level" class designed to be used internally by clients. Also the stream() might be misleading as a name, as it is now only getting you a GrpcStreamBroadcaster.
Another option would be to just accept the same arguments as GrpcStreamBroadcaster.new_receiver() in your stream() method.
On the other hand, if we return a streamer, it means less code duplication, and having a common interface to "configuring" the receiver for all streaming methods. If we want to go that route, maybe we can create a Protocol to only expose the parts of the GrpcStreamBroadcaster we want to be public. We even already have something like this, the ReceiverFetcher, but it more general, so I think the new_receiver() it takes no arguments (or maybe only the buffer size).
In any case, I think we need to talk about this more broadly, so all clients provide the same interface. If one client returns a GrpcStreamBroadcaster and another one returns a plain receiver, then our clients will become inconsistent.
This allows us to use all the options available in the streamer.