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

fix event lost #1083

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ningmingxiao
Copy link

@ningmingxiao ningmingxiao commented Aug 20, 2024

sometimes if listener chan is full,event will lost.
@fsouza @bufdev
can you review my pr? thank you

@ningmingxiao ningmingxiao force-pushed the print_event_lost branch 3 times, most recently from b932fb8 to e2dd0d9 Compare August 20, 2024 09:10
@ningmingxiao ningmingxiao changed the title add some log print event lost fix event lost Aug 20, 2024
@bufdev
Copy link
Contributor

bufdev commented Aug 28, 2024

@ningmingxiao I have not been involved in this repo for about a decade, it's not really professional to tag people here. Will leave this to others.

@ningmingxiao
Copy link
Author

sory,but thank you anyway.

@fsouza
Copy link
Owner

fsouza commented Sep 22, 2024

Thanks for contributing! This is a big change that can lead to slow consumers hanging. I recommend either making sure your caller is always fast enough to consume the events, or putting the new behavior behind a knob.

@ningmingxiao
Copy link
Author

ningmingxiao commented Sep 23, 2024

sometimes I can't make sure consumers run faster, if run plenty of containers, will generate many events, channel will be filled. At least we should let user know event is dropped when chan is filled.

@fsouza
Copy link
Owner

fsouza commented Oct 28, 2024

Can you add a knob in that case?

@ningmingxiao
Copy link
Author

Can you add a knob in that case?

Could you describe it in more detail? Thank you

@fsouza
Copy link
Owner

fsouza commented Oct 29, 2024

@ningmingxiao I imagine we'd have a new option when adding the listener. Internally the listener would become some struct with the channel + some listening options, and externally we'd probably need to have versions of AddEventListenerWithOptions and RemoveEventListener that take a struct with "listening options" instead of just the channel.

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