Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Apr 28, 2025

No description provided.

Copilot AI review requested due to automatic review settings April 28, 2025 13:42
@Marenz Marenz requested review from a team as code owners April 28, 2025 13:42
@Marenz Marenz requested a review from llucax April 28, 2025 13:42
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:dispatcher labels Apr 28, 2025
Copy link

Copilot AI left a 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 fixes the stream reconnecting issue while updating the stream() API to now return a GrpcStreamBroadcaster instead of a receiver. In addition, the release notes have been updated to document this breaking change and the new retry strategy support.

  • Updated test to call .new_receiver() after calling stream() on the client.
  • Modified the client implementation to accept an optional stream_retry_strategy and return a GrpcStreamBroadcaster.
  • Updated release notes to reflect the changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/test_client.py Test updated to append .new_receiver() to support the new stream() return.
src/frequenz/client/dispatch/_client.py Updated stream() return type and implemented the new stream retry strategy.
RELEASE_NOTES.md Added release notes for the breaking change and new stream retry strategy.

@Marenz Marenz enabled auto-merge April 28, 2025 13:43
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 didn't have a look at the code in detail yet, but...

I don't understand if the fix is because of the addition of the retry strategy option, because of the returning of the streamer or both. Also I don't understand how this solves the reconnection problem. It would be nice to clarify in the PR.

Also, unless you really need something from the broadcaster as a user, I would return a ReceiverFetcher instead, so we can keep the interface a bit more abstract and not leak the internals, otherwise the streamer class also becomes part of the interface, which needs a 1.0 from client base too.

About the interface change, this is still at 0.x so I don't see why we can't make breaking changes...

@Marenz
Copy link
Contributor Author

Marenz commented Apr 29, 2025

Well, the fox is because of the commit starting with "Fix:" :) where as the other commit is the breaking change..

@Marenz
Copy link
Contributor Author

Marenz commented Apr 29, 2025

I am not sure, a ReceiverFetcher only lets me get a receiver, I can't check if the channel is closed, manually close the connection or any of the other things the Grpcbroadcaster offers...

@Marenz
Copy link
Contributor Author

Marenz commented Apr 29, 2025

I removed the interface change now as it seems to require discussion and we need this fix urgently.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz requested a review from llucax April 29, 2025 08:13
@llucax llucax added the scope:breaking-change Breaking change, users will need to update their code label Apr 29, 2025
@llucax
Copy link
Contributor

llucax commented Apr 29, 2025

I am not sure, a ReceiverFetcher only lets me get a receiver, I can't check if the channel is closed, manually close the connection or any of the other things the Grpcbroadcaster offers...

Well, that was my question. The thing is GrcpBroadcaster was thought as an internal thing, so most "services" provided by it were done for the people implementing clients, not the end users of clients. I don't want to expose too much stuff to the end user because then we need to commit to the interface.

I guess you mean manually close the stream, not the connection, right? Also looking at the streamer class, it only has a stop(), it doesn't provide any way to know if the stream is still open or not. I'm not sure we want to let users close streams, they might not be the only user of the stream, we have to do that with care. If they want to "unsubscribe" they should just close() the receiver.

The more I look at it, the more I think there is no point in exposing the streamer. If we need to allow the user to check if a stream is alive, then we need to first implement that somehow, just exposing the internal streamer is not helping.

But yeah, returning a ReceiverFetcher doesn't make sense either, I think we should keep it as it is (return Receiver).

@llucax llucax requested a review from Copilot April 29, 2025 10:30
Copy link

Copilot AI left a 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 fixes the stream reconnection issue by updating the retry strategy and adjusts the stream() method to return the streamer object instead of the receiver object. It also updates the release notes to document these changes.

  • Updated retry strategy in _client.py to allow unlimited reconnection attempts.
  • Modified the stream() method behavior to adhere to new functionality.
  • Added an entry in RELEASE_NOTES.md to detail the changes.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/frequenz/client/dispatch/_client.py Updated retry strategy to use unlimited retries for reconnection
RELEASE_NOTES.md Documented the stream reconnection fix
Comments suppressed due to low confidence (1)

src/frequenz/client/dispatch/_client.py:244

  • Changing 'limit' from 0 to None will result in unlimited retry attempts. Verify that unlimited retries are intended here and that the LinearBackoff implementation handles a None value correctly.
retry_strategy=LinearBackoff(interval=1, limit=None)

@Marenz Marenz added this pull request to the merge queue Apr 29, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 207b4e9 Apr 29, 2025
6 checks passed
@Marenz Marenz deleted the fix-recon branch April 29, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:dispatcher part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests scope:breaking-change Breaking change, users will need to update their code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants