Skip to content

Conversation

@Prunkles
Copy link
Contributor

@Prunkles Prunkles commented Oct 17, 2024

There is no way to detect when an event is dropped due to BufferOptions.BoundedChannelFullMode being set to drop.

Starting with some version of System.Threading.Channels, an itemDropped callback was added to the BoundedChannel. So I added a BufferItemDropped callback to the ChannelOptionsBase.

This callback would fit better in the BufferOptions, but because it requires a TEvent, it would also need to be added to the BufferOptions. I am not sure if it is affordable, because of how many things depend on BufferOptions being without generic parameters.

@cla-checker-service
Copy link

cla-checker-service bot commented Oct 17, 2024

💚 CLA has been signed

@Prunkles
Copy link
Contributor Author

I guess I've signed the CLA, but I haven't received any confirmation

@Prunkles
Copy link
Contributor Author

Oh, never mind, it updated when I commented

@Prunkles Prunkles force-pushed the feat/add-item-dropped branch from 1da3590 to 81601a1 Compare November 19, 2024 15:46
@stevejgordon
Copy link
Contributor

Sorry for the delay, @Prunkles . Thanks for contributing this PR. In principle, it's okay and something we should add, but I'd like to consider where we should include this option a bit further.

@Prunkles
Copy link
Contributor Author

@stevejgordon

I'd like to consider where we should include this option a bit further.

This decision was made based on the Channel.CreateBounded(BoundedChannelOptions options, Action<T>? itemDropped) itself: as we can see, it takes itemDropped separately from other options as well. I guess it is for the same reason I wrote above (to avoid making the Options type generic)

This callback would fit better in the BufferOptions.

Also, I thought about it a bit more, and I think I was wrong, at least because that itemDropped option is clearly intended for the BCL Channel type, so it should be in the corresponding ChannelBufferOptions, and not just BufferOptions which does not have any "channel" semantics

So I think the current placement is the most appropriate

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks @Prunkles !

@Mpdreamz Mpdreamz merged commit 14de339 into elastic:main Feb 11, 2025
6 checks passed
@Prunkles Prunkles deleted the feat/add-item-dropped branch February 11, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants