Skip to content

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 <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
@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