-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[IMPROVED] Multi-Filtered Consumers #5274
Conversation
Signed-off-by: Derek Collison <[email protected]>
…ects. Signed-off-by: Derek Collison <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but wonder if we should consider #5275 for testing new functionality like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Though I have one doubt which I commented about.
// Check if we are multi-filtered or not. | ||
if o.filters != nil { | ||
sm, sseq, err = store.LoadNextMsgMulti(o.filters, o.sseq, &pmsg.StoreMsg) | ||
} else if o.subjf != nil { // Means single filtered subject since o.filters means > 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite few implicit assumptions and relations here:
- That
o.filters == nil
means single or none filtesrs. - That
o.subjf != nil
means signel filter
Thoes are fine, but it's easy to introduce bugs in the future by changing how we update consumers, filters etc.
This change introduces a new LoadNextMsgMulti into the store layer. It is passed a sublist that is maintained by the consumer. The store layer matches potential messages across any positive match in the sublist.
Resolves: #4888
Signed-off-by: Derek Collison [email protected]