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

Support for disabling overflow-warning logs in broadcast receivers #329

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

shsms
Copy link
Contributor

@shsms shsms commented Oct 29, 2024

No description provided.

shsms added 2 commits October 29, 2024 14:17
This is required in cases where users are only interested in the
latest value, for example.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms shsms requested a review from a team as a code owner October 29, 2024 13:23
@github-actions github-actions bot added part:docs Affects the documentation part:channels Affects channels implementation labels Oct 29, 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.

Not super excited about the extra complexity, and I was thinking it would be great to manage this via the config system, but even if we have a separate logger for these warnings so it can be disabled more easily, this doesn't allow us to show warnings for some receivers but not others, so I guess it doesn't cut it.

My main concern is I guess we are adding this only to cope with the fact that we can't unsubscribe/remove receivers, right? Or am I missing any other use case for this? It seems like having a channel overflowing is not good unless you don´t really care about receiving data from it.

@shsms
Copy link
Contributor Author

shsms commented Oct 29, 2024

It is for cases where we want just the latest value, but also want a trigger, want to put it in a select, etc.

@llucax
Copy link
Contributor

llucax commented Oct 29, 2024

OK, and in that case it will be combined with size=1 I guess. Maybe it is worth creating a different receiver for this use case? Maybe new_xxx_receiver() where I have no idea what xxx could be 😆

@shsms
Copy link
Contributor Author

shsms commented Oct 31, 2024

How about a private method Receiver._allow_overflows, while we decide the public interface?

@llucax llucax added this to the v1.3.0 milestone Nov 1, 2024
@llucax
Copy link
Contributor

llucax commented Nov 1, 2024

Unless having this is urgent, I'd rather spend some time thinking about a final solution than introducing a hack. And if we are going for the hack, I would be more inclined to have _warn_on_overflow: bool as an argument to new_receiver() and mention in the docs that this is experimental. But I would prefer not to start making every feature experimental from now on either 😆

@llucax
Copy link
Contributor

llucax commented Nov 21, 2024

Moving the discussion to:

To avoid polluting the PR too much.

@shsms shsms added this pull request to the merge queue Dec 9, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 565084d Dec 9, 2024
14 checks passed
@shsms shsms deleted the no-overflow-warning branch December 9, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:channels Affects channels implementation part:docs Affects the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants