[ADDED] Support for multiple subject filters on consumers #1214

piotrpio · 2023-02-20T10:28:46Z

Addresses nats-io/nats-architecture-and-design#196

coveralls · 2023-02-20T10:33:48Z

Coverage: 85.359% (+0.03%) from 85.33% when pulling 9859354 on multiple-filter-subjects into 5931be0 on 2.10.0.

wallyqs · 2023-02-25T04:59:38Z

jsm.go

@@ -408,6 +412,11 @@ func (js *js) upsertConsumer(stream, consumerName string, cfg *ConsumerConfig, o
 		}
 		return nil, info.Error
 	}
+
+	// check whether multiple filter subjects (if used) are reflected in the returned ConsumerInfo
+	if len(cfg.FilterSubjects) != 0 && len(info.Config.FilterSubjects) == 0 {


To test this I guess would need to run against a version of the server that does not support filter subjects 🤔

Yup, this is basically server compatibility check.

wallyqs · 2023-02-25T05:12:21Z

jsm.go

+
+	// check whether multiple filter subjects (if used) are reflected in the returned ConsumerInfo
+	if len(cfg.FilterSubjects) != 0 && len(info.Config.FilterSubjects) == 0 {
+		return nil, ErrConsumerMultipleFilterSubjectsNotSupported


At this point I think it would have have created the consumer but instead returning an error? Wondering how we should we handle creating durable consumers unintentionally that are not used, or document in the error that may have to cleanup the consumer that got created (or return both consumer info and error?).

Yeah, we were wondering how to best handle this scenario, IMO it's better to document + return an error than to fail silently and create the consumer with invalid state. I will expand on the error documentation.

wallyqs · 2023-02-25T05:12:31Z

test/js_test.go

+		js.Publish("baz", []byte("msg"))
+	}
+
+	sub, err := js.SubscribeSync("", nats.BindStream("TEST"), nats.ConsumerFilterSubjects("foo", "baz"))


is this an ephemeral consumer? do we need another one with a durable name to cover some of the conditions above?

Jarema

Looks good, with some small nits.

Jarema · 2023-02-24T13:45:08Z

jsm.go

@@ -375,9 +375,13 @@ func (js *js) upsertConsumer(stream, consumerName string, cfg *ConsumerConfig, o
 		ccSubj = fmt.Sprintf(apiLegacyConsumerCreateT, stream)
 	} else if err := checkConsumerName(consumerName); err != nil {
 		return nil, err
-	} else if !js.nc.serverMinVersion(2, 9, 0) || (cfg.Durable != "" && js.opts.featureFlags.useDurableConsumerCreate) {
+	} else if !js.nc.serverMinVersion(2, 9, 0) ||


Suggested change

} else if !js.nc.serverMinVersion(2, 9, 0) ||

} else if !js.nc.serverMinVersion(2, 10, 0) ||

This check should be for 2.9.0 as it's about verifying if we can use subject with filter subject token when creating consumer (which was 2.9.0)

jsm.go

Jarema

LGTM!

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

tpihl · 2023-07-02T09:03:29Z

@piotrpio I am trying to understand where our new is heading and looking at this pr, It's something I don't get;

We have a new error if filtersubjects is empty? Why?

TLDR; I think it makes more sense to allow it to be empty, and that is an indication that the consumer should not filter the stream, should get all.

In a little more detail;

If i want to read everything in the stream, do I now have to add '>'. And is that allowed or will that be illegal since it's outside the streams defined subjects.. If it's illegal, do I need to mirrior every subject from the stream definition to get all?

The concept of filtering for me is "no filters means everything". And then filters should possibly be of both types to be able to widen the filter or narrow the filter. To mimic current and my understanding if intended behavior but with the filter-perspective instead of what i would call subset-if-subjects or only-subjects;

empty filtering -> get everything
onlySubjects(subjectwithwildcard,...) -> only msgs with subject matching provided filters
exceptSubjects(subjectswithcildcards...) -> only msgs with subject NOT matching provided filter

Collisions would be avoided by a few simple sorting-criteria so that more explicit filter-parts wins over those with wildcards.

piotrpio · 2023-07-02T11:52:04Z

@tpihl The error will not be returned if you provide empty or nil slice, only if you give an empty string as an element of the slice:

FilterSubjects: []string{} - valid
FilterSubjects: nil - valid
FilterSubjects: []string{""} - invalid

That's a safeguard on the server, and it's treated as invalid configuration. I do not see the point of providing empty string as filter value (in slice), that looks more like unintentional behavior.

The concept of filtering for me is "no filters means everything".

You're right about this - but there is a difference between providing empty slice (no filters) and providing an empty string as filter.

tpihl · 2023-07-02T18:56:52Z

Okay, was just reading the client code, and didn't understand the error. Maybe something like "filtersubjects cannot contain an empty string" would have been clearer?

/T

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio requested a review from Jarema February 20, 2023 10:28

piotrpio marked this pull request as draft February 20, 2023 10:39

piotrpio force-pushed the multiple-filter-subjects branch from 4682c73 to 3f37c97 Compare February 20, 2023 11:04

piotrpio marked this pull request as ready for review February 20, 2023 11:04

piotrpio requested a review from wallyqs February 20, 2023 14:44

piotrpio changed the base branch from main to 2.10.0 February 24, 2023 14:15

wallyqs reviewed Feb 25, 2023

View reviewed changes

Jarema requested changes Mar 1, 2023

View reviewed changes

bojanz mentioned this pull request Jun 5, 2023

FilterSubjects does not work #1294

Closed

piotrpio force-pushed the multiple-filter-subjects branch from 9859354 to 36db4fe Compare June 14, 2023 04:22

piotrpio force-pushed the 2.10.0 branch from 5931be0 to 7a40b70 Compare June 14, 2023 04:23

piotrpio force-pushed the multiple-filter-subjects branch from b65cadc to 16c3563 Compare June 14, 2023 06:19

piotrpio requested a review from Jarema June 14, 2023 06:19

Jarema approved these changes Jun 14, 2023

View reviewed changes

piotrpio added 5 commits June 14, 2023 08:53

[ADDED] Support for multiple subject filters on consumers

c584091

Check for empty filter subjects using len()

8ea3e31

Fix ErrConsumerMultipleFilterSubjectsNotSupported comment

1cdc344

Add test for durable consumer

5a6f722

Add multiple filter subjects support in new API

6f515e7

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio force-pushed the multiple-filter-subjects branch from 16c3563 to 6f515e7 Compare June 14, 2023 06:53

piotrpio merged commit 4eac72b into 2.10.0 Jun 14, 2023

piotrpio deleted the multiple-filter-subjects branch June 14, 2023 07:34

jnmoyne pushed a commit that referenced this pull request Aug 9, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

7b11949

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio added a commit that referenced this pull request Aug 26, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

fd6148b

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio added a commit that referenced this pull request Sep 1, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

4f3bd4e

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio added a commit that referenced this pull request Sep 1, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

c36caee

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio added a commit that referenced this pull request Sep 19, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

58918df

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio added a commit that referenced this pull request Sep 19, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

309d269

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio added a commit that referenced this pull request Sep 20, 2023

[ADDED] Support for multiple subject filters on consumers (#1214)

8ca661b

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>

piotrpio mentioned this pull request Sep 20, 2023

Release v1.30.0 #1411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADDED] Support for multiple subject filters on consumers #1214

[ADDED] Support for multiple subject filters on consumers #1214

piotrpio commented Feb 20, 2023

coveralls commented Feb 20, 2023 •

edited

Loading

wallyqs Feb 25, 2023

piotrpio Feb 26, 2023

wallyqs Feb 25, 2023

piotrpio Feb 26, 2023 •

edited

Loading

wallyqs Feb 25, 2023

Jarema left a comment

Jarema Feb 24, 2023

piotrpio Jun 14, 2023

Jarema left a comment

tpihl commented Jul 2, 2023

piotrpio commented Jul 2, 2023

tpihl commented Jul 2, 2023

	} else if !js.nc.serverMinVersion(2, 9, 0) \|\|
	} else if !js.nc.serverMinVersion(2, 10, 0) \|\|

[ADDED] Support for multiple subject filters on consumers #1214

[ADDED] Support for multiple subject filters on consumers #1214

Conversation

piotrpio commented Feb 20, 2023

coveralls commented Feb 20, 2023 • edited Loading

wallyqs Feb 25, 2023

Choose a reason for hiding this comment

piotrpio Feb 26, 2023

Choose a reason for hiding this comment

wallyqs Feb 25, 2023

Choose a reason for hiding this comment

piotrpio Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

wallyqs Feb 25, 2023

Choose a reason for hiding this comment

Jarema left a comment

Choose a reason for hiding this comment

Jarema Feb 24, 2023

Choose a reason for hiding this comment

piotrpio Jun 14, 2023

Choose a reason for hiding this comment

Jarema left a comment

Choose a reason for hiding this comment

tpihl commented Jul 2, 2023

piotrpio commented Jul 2, 2023

tpihl commented Jul 2, 2023

coveralls commented Feb 20, 2023 •

edited

Loading

piotrpio Feb 26, 2023 •

edited

Loading