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

Improve Consumer with multiple subjects #4188

Merged
merged 3 commits into from Jun 1, 2023
Merged

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented May 23, 2023

While adding more complex test scenarios, some issues in multiple-filters approach surfaced.
This PR adds those tests and addresses the issues.

Signed-off-by: Tomasz Pietrek tomasz@nats.io

@Jarema Jarema force-pushed the jarema-work-on-multiple-filters branch 3 times, most recently from 6cc9b96 to 33c44c7 Compare May 23, 2023 08:08
@Jarema Jarema marked this pull request as ready for review May 27, 2023 05:50
@Jarema Jarema requested a review from a team as a code owner May 27, 2023 05:50
server/consumer.go Outdated Show resolved Hide resolved
server/consumer.go Show resolved Hide resolved
server/consumer.go Show resolved Hide resolved
@derekcollison derekcollison self-requested a review May 31, 2023 14:49
@@ -3247,7 +3252,7 @@ func (o *consumer) getNextMsg() (*jsPubMsg, uint64, error) {
store := o.mset.store

// If no filters are specified, optimize to fetch just non-filtered messages.
if o.subjf == nil {
if o.subjf == nil || len(o.subjf) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Just do len() since len(nil) is valid.

@@ -3313,31 +3315,36 @@ func (o *consumer) getNextMsg() (*jsPubMsg, uint64, error) {
// to avoid reflection.
if o.subjf != nil && len(o.subjf) > 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, just do len.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@Jarema Jarema force-pushed the jarema-work-on-multiple-filters branch from ed52a36 to 1c4f664 Compare June 1, 2023 06:27
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
@Jarema Jarema force-pushed the jarema-work-on-multiple-filters branch from 1c4f664 to 261f39b Compare June 1, 2023 06:29
@Jarema Jarema changed the title Improve multiple subjects handling Improve Consumer with multiple subjects Jun 1, 2023
@Jarema Jarema merged commit 7b74f4f into dev Jun 1, 2023
2 checks passed
@Jarema Jarema deleted the jarema-work-on-multiple-filters branch June 1, 2023 07:14
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.

None yet

2 participants