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

Add an optional filterFn to PubSubAsyncIterator to workaround performance issues found in withFilter #601

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

Conversation

davisg123
Copy link

Our production application filters a high number of messages, more than 99% are filtered out. We suffered an issue of unbounded memory growth caused by the behavior of withFilter. Our understanding is that withFilter will continue chaining promises together until a message is accepted. Because of our aggressive filtering our containers would quickly run out of memory

Screenshot%202023-05-01%20at%2011 43 15%20AM

We have since improved our sharding behavior so that we are filtering less messages, but we still think there is an improvement that can be made to filtering.

This PR adds a filterFn to PubSubAsyncIterator so that messages can be filtered immediately before insertion into the queue. We believe this is a performance win regardless of how many messages are being filtered. Thank you!

@davidyaha
Copy link
Owner

Hey @davisg123!
Thanks for the PR and sorry for my late review.

I look at the example provided and I can't help but wondering why the event is even published in the first place?
This looks like not the responsibility of something like asnycIterator on it's own. The whole point is to allow chaining of filters so that there is a "stream-like" behaviour rather than a simple "one filter rules all" approach.

Could you perhaps suggest a more complex example where this filter function logic cannot be applied before calling the publish method?

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.

2 participants