allow max consumers to be set

[ripienaar]

Signed-off-by: R.I.Pienaar rip@devco.net

/cc @nats-io/core

[ripienaar]

@derekcollison this fixes nats-io/jetstream#269, however it still fails because:

	if config.MaxConsumers > 0 && jsa.limits.MaxConsumers > 0 && config.MaxConsumers > jsa.limits.MaxConsumers {
		return fmt.Errorf("maximum consumers exceeds account limit")
	} else {
		config.MaxConsumers = jsa.limits.MaxConsumers
	}

Now we hit the else which always force config.MaxConsumers to equal the limit, I dont understand the motivation behind that so not really sure what the fix is here.

For sure if a client asks for 10 max consumers, we should set 10 max consumers if limits allow. And if the limit does not allow that we should error.

We shouldnt change what they set behind the scenes as that will break all config management tools who work on desired state outcomes - the desired state can never match the outcome because we ignore the desired state.

[derekcollison]

I think the else clause should be

	} else if jsa.limits.MaxConsumers > 0 {
		config.MaxConsumers = jsa.limits.MaxConsumers
	}

[ripienaar]

Even then though, why would the configured limit override what the client configures for this specific stream? Shouldn't that be some max that cannot be exceeded but less than it is fine?

[derekcollison]

True, so just remove the else clause then.

[derekcollison]

But if they set it to 0 and the jsa limit is set, we should set it to that IMO..

[ripienaar]

The problem with overwriting their desire is that if you tell, for example terraform, limit 0 and later it asks what the config is and it does a diff against requested and crurent it will then try to remediate.

Now in this specific instance thats not possible since max consumers cannot be changed, but I think its a bad pattern in general to not trust users and not give them feedback that they did something weird. So much rather fail to create a stream with unlimited consumers than force them to some other limit which might break their app.

[derekcollison]

Yes I think we just remove the else clause..

[ripienaar]

OK, I killed the else, thank you

[derekcollison]

LGTM

[ripienaar]

Failure in tests gets us to the discussion about what to do with 0, because the test didnt set max consumers and relied instead on the behaviour of max being inherited.

Even with the stream set to unlimited and the account set to 10 the stream shouldnt be added because that exceeds the limit. So let me see where I can add that constraint

[derekcollison]

Well two options if the jsa.limit is set and config is not, inherit it or fail.

Or we could make sure that we check both during runtime.

[ripienaar]

Either is fine, I think checking both at runtime is required regardless, so I'll go with just doing that.

Later I imagine we might support changing account limits - then all streams should just continue to work that was previously set to unlimited, if we inherited we'd have to go update all the stream configs when we do that.

[ripienaar]

I've done an update to allow unlimited streams to be overruled by the account limits for max consumers, I dont think we have this scenario on other limits so this should probably put it to bed

[ripienaar]

would these require any account lock here?

[derekcollison]

Possibly for the jsa.limits. I think we have a protected method to get the limits with a readlock. Would just do that, they are copied IIRC.

[derekcollison]

LGTM

[derekcollison]

Possibly for the jsa.limits. I think we have a protected method to get the limits with a readlock. Would just do that, they are copied IIRC.