Adjust consumer creation to nats-server v2.9.0 #1080

piotrpio · 2022-09-14T13:49:58Z

Resolves #1077

coveralls · 2022-09-14T13:54:46Z

Coverage increased (+0.1%) to 85.674% when pulling 069041d on consumer-create-updates into 25b6392 on main.

jserrors.go

Co-authored-by: Waldemar Quevedo <wally@synadia.com>

test/js_test.go

wallyqs

LGTM

Co-authored-by: Waldemar Quevedo <wally@synadia.com>

kozlovic · 2022-09-14T15:46:31Z

jsm.go

+	if cfg == nil {
+		cfg = &ConsumerConfig{}
+	}
+	if cfg.Name != _EMPTY_ && !js.nc.serverMinVersion(2, 9, 0) {


I implemented the C client and did not do the version check. Derek commented on that PR that this is not necessary: nats-io/nats.c#580

kozlovic · 2022-09-14T15:49:25Z

jsm.go

+	} else if err := checkConsumerName(consumerName); err != nil {
+		return nil, err
+	} else if js.nc.serverMinVersion(2, 9, 0) {
+		// if above server version 2.9.0, use the endpoints with consumer name


If not checking the server name, what I think should be done is just in checking if the new config Name is specified. If it is, then that signals the intent to use the new API endpoints. If user specifies Name, but connects to an older server, they will get a timeout: this is not ideal, but I don't think checking server version is reliable anyway (could be connected to one 2.9.0, but server accepting the request be older version, or vice versa).

Makes sense, I'll change it.

So, just to clarify - when user provides only Durable, we should use the old endpoints (CONSUMER.DURABLE.CREATE)?
If so, that makes it problematic for e.g. Subscribe("foo", nats.Durable("cons")), as basically it will always use old API subject, unless we add another SubOpt for just setting consumer name.

We should then possibly set the Name to the Durable before calling js.AddConsumer(). But then, I agree that we have the situation where this is not a user choice and so connecting to older server would be a problem...

So from what I understand, we have following options:

Not verify the server version compatibility and call the new API if Name is set - problem here is, Subscribe() will always call the old API when combined with Bind()/Durable() options

Do as above, but set Name to the value of Durable in Subscribe() - that means however that we would always be using new API, even for older servers (that's not an option)

Verify server version when choosing the right subject in upsertConsumer() - here, we might have an issue of connecting to an older server, as you described in the original comment.

Looking at those, I would still lean towards option 3 - I could strip version check in AddConsumer()/UpdateConsumer(), but leave the version check when choosing the right subject in upsertConsuper(). What do you think?

The pure version orientated approach means users do not get a chance to say they need time to update infrastructure like ACLs and so forth to start using it.

Hm, not sure how big of an issue that would be vs forcing the user to intentionally start using the new API by changing their application code (IMO this transition should ideally be seamless, as the user of client library I don't need to care about the API subjects). What do you think?

There is a class of user who care deeply to lock down the APIs according to needs. Also a class of user who have spent considerable time in navigating the API subjects for cross account use via imports and exports.

A major design feature of these API subjects is to enable that lock down. Or to be selectively imported to manage permissions and restrictions.

The Venn diagrams of users likely to pay for NATS and those who care deeply for subject security probsbly has quite a lot of overlap.

So you might not care, but I think we should consider if introducing new features in the most user hostile way possible is perhaps not the right thing - especially considering the users most likely affected.

The issue with the ACL is possibly real, but in reality, the same logic can be applied the other way. If they deploy the new client but don't update the servers, they can update their ACLs and then deploy the servers.

The client possibly could have a way of rejecting the use of the new API (I do for test purposes). But at some point, the clients become too complex and too flexible.

The question is how do users know? Do we have effective communication channels to tell people about these changes? Release notes and blog post are notoriously ineffective - especially as ours tend to be enormous.

So do we feel users are served well enough by the communication and warning we give them about upcoming changes?

In another world these would be considered breaking changes and tooling and just human behaviour is aware of major changes. These are not just new features. They majorly change existing code simply because it happens to point at another version server.

As much as I loathe the go major version change behaviour this does demonstrate the utility of that.

jserrors.go

jsm.go

wallyqs · 2022-09-14T16:12:39Z

jsm.go

+		cfg = &ConsumerConfig{}
+	}
+	if cfg.Name != _EMPTY_ && !js.nc.serverMinVersion(2, 9, 0) {
+		return nil, fmt.Errorf("%w: %s", ErrRequireServerVersion, "consumer name requires at least server version 2.9.0")


not for this PR maybe but we should be doing this style of error wrapping with the ErrRequireServerVersion in other places too like:

nats.go/kv.go

Line 309 in cc189da

return nil, errors.New("nats: key-value requires at least server version 2.6.2")

kozlovic

LGTM

scottf · 2022-09-15T18:52:50Z

If anyone is interested, this is what I do and I think Alberto does pretty much the same:

1. If name is provided and server is not 2.9.0 or later, error
2. if name and durable are provided they must match
3. consumer name is either name or durable, whichever is provided. Both can be blank

if consumer name is blank against any server version, use CONSUMER.CREATE.{{stream}}
else if server is 2.9.0 or later
   if filter subject is blank or '>' use CONSUMER.CREATE.{{stream}}.{{consumerName}}
   else use CONSUMER.CREATE.{{stream}}.{{consumerName}}.{{filterSubject}}
else, since consumer name is not blank (so durable) and server is older than 2.9.0, use CONSUMER.DURABLE.CREATE.{stream}.{consumerDurableName}

aricart

LGTM

ripienaar · 2022-09-16T07:12:51Z

js.go

+// UseLegacyDurableConsumers makes JetStream use the legacy (pre nats-server v2.9.0) subjects for consumer creation.
+// If this option is used when creating JetStremContext, $JS.API.CONSUMER.DURABLE.CREATE.<stream>.<consumer> will be used
+// to create a consumer with Durable provided, rather than $JS.API.CONSUMER.CREATE.<stream>.<consumer>.
+func UseLegacyDurableConsumers() JSOpt {


Seems like a good compromise to me, allows people to opt out of a feature should they be in a situation where server updates isnt possible.

Of course in other projects this is opt in -> opt out -> remove flag over 3 releases. But seems this is the best we can do at present.

The main issue with this is that the default client behavior is still to use the new feature. So really they have to change code, so they could also hold upgrading the server or the client. If there's a flag, my guess is it should be opt-in, rather than opt-out.

My preference would be an opt-in also.

To your point "they could also hold upgrading the server"....this is just not the case, our default answer to any support question is "upgrade the server", users might not be in a position to cange code etc, hence why I prefer opt-in.

I guess the way these type of changes are rolled out is to wait for a few releases before making the default if there is no alternative, I like the feature flags approach from the PR, maybe we could have eventually an option for the JetStream context to always use latest features instead or to enable a subset of them.

nc.JetStream(nats.UseLatestJSFeatures()) // all opt-in nc.JetStream(nats.UseLegacyDurableConsumers()) // opt-out nc.JetStream(&nats.JetStreamFeatures{LegacyDurableConsumers: true, AllowDirect: true})

UseLatestJSFeatures() would be a nice addition in the future for sure. I would probably lean towards merging this PR as is (with having the opt-out option if needed, to be deprecated and removed in future releases), not to change the behavior vs other clients right now. If we decide we want to have future features like this opt-in, we need to be consistent across all clients. As some of the clients are already released without a feature flag, I don't think it's good to have this discrepancy.

kozlovic

LGTM

wallyqs

LGTM

Adjust consumer creation to nats-server v2.9.0

8842548

piotrpio requested review from wallyqs, kozlovic and aricart September 14, 2022 13:50

piotrpio requested a review from Jarema September 14, 2022 14:02

wallyqs reviewed Sep 14, 2022

View reviewed changes

jserrors.go Outdated Show resolved Hide resolved

Update jserrors.go

7d21454

Co-authored-by: Waldemar Quevedo <wally@synadia.com>

wallyqs reviewed Sep 14, 2022

View reviewed changes

test/js_test.go Outdated Show resolved Hide resolved

wallyqs approved these changes Sep 14, 2022

View reviewed changes

piotrpio and others added 2 commits September 14, 2022 16:28

Update test/js_test.go

2bdb6d0

Co-authored-by: Waldemar Quevedo <wally@synadia.com>

Fix typo

c101ec9

kozlovic requested changes Sep 14, 2022

View reviewed changes

wallyqs reviewed Sep 14, 2022

View reviewed changes

piotrpio added 2 commits September 14, 2022 20:21

Remove ErrConsumerNameMismatch check

583e80c

Get rid of compatibility check in create / update consumer

5c4567f

kozlovic approved these changes Sep 15, 2022

View reviewed changes

aricart approved these changes Sep 15, 2022

View reviewed changes

Add UseLegacyDurableConsumers() option

069041d

ripienaar reviewed Sep 16, 2022

View reviewed changes

kozlovic approved these changes Sep 16, 2022

View reviewed changes

wallyqs approved these changes Sep 16, 2022

View reviewed changes

piotrpio merged commit 3c209f5 into main Sep 16, 2022

piotrpio deleted the consumer-create-updates branch September 16, 2022 15:46

piotrpio mentioned this pull request Sep 22, 2022

Changes in Durable/Ephemeral creation nats-io/nats-architecture-and-design#148

Open

13 tasks

marthaCP mentioned this pull request Jan 18, 2023

Implement Allow consumer filter subject to be updated nats-io/nats-pure.rb#105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust consumer creation to nats-server v2.9.0 #1080

Adjust consumer creation to nats-server v2.9.0 #1080

piotrpio commented Sep 14, 2022

coveralls commented Sep 14, 2022 •

edited

wallyqs left a comment

kozlovic Sep 14, 2022

kozlovic Sep 14, 2022

piotrpio Sep 14, 2022

piotrpio Sep 14, 2022

kozlovic Sep 14, 2022

piotrpio Sep 14, 2022

ripienaar Sep 15, 2022

piotrpio Sep 15, 2022

ripienaar Sep 15, 2022

aricart Sep 15, 2022

ripienaar Sep 15, 2022

wallyqs Sep 14, 2022

kozlovic left a comment

scottf commented Sep 15, 2022 •

edited

aricart left a comment

ripienaar Sep 16, 2022

aricart Sep 16, 2022

ripienaar Sep 16, 2022

wallyqs Sep 16, 2022 •

edited

piotrpio Sep 16, 2022

kozlovic left a comment

wallyqs left a comment

Adjust consumer creation to nats-server v2.9.0 #1080

Adjust consumer creation to nats-server v2.9.0 #1080

Conversation

piotrpio commented Sep 14, 2022

coveralls commented Sep 14, 2022 • edited

wallyqs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

scottf commented Sep 15, 2022 • edited

aricart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyqs Sep 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

wallyqs left a comment

Choose a reason for hiding this comment

coveralls commented Sep 14, 2022 •

edited

scottf commented Sep 15, 2022 •

edited

wallyqs Sep 16, 2022 •

edited