Allow setting consumer replicas though options

[goku321]

Currently, only js.AddConsumer supports configuring replicas count for a consumer. Configuring it through nats.SubOpt will allow configuring the same with js.Subscribe, js.PullSubscribe etc.

[coveralls]

Coverage decreased (-0.005%) to 84.08% when pulling 1d9feeb on goku321:opt-cons-replica into f4a86f3 on nats-io:main.

[piotrpio]

Hey, this looks good, I just have 2 suggestions:

1. Could you add a simple check in checkConfig() func in js.go to verify whether the value is not changed if the consumer already exists (this is done for other settable parameters as well to avoid overwriting existing config in Subscribe()
2. It would be nice to have at least a simple test in test/js_test.go to verify whether a consumer created with Subscribe() has a proper Replicas value set in config.

[piotrpio]

I think we can also consider setting adding && replicas <=5, as that is the upper limit.

[derekcollison]

That is the current server limit, but since server defined and we could change would leave that out..

[kozlovic]

@piotrpio @derekcollison @tbeets I may be mistaken, but the approach of the JS simplification is that we would no longer create JS consumers, so not sure if it is advisable to add those sub options that would no longer be needed after the upcoming simplification?

[piotrpio]

@kozlovic Simplified JestStream API will be in it's own package, so the current implementation can be modified separately.
You're right, the simplified API will not allow creating consumers at the time of subscribing, so there will be no need for adding those options.
Here is the draft of js-simplification - #1018

[kozlovic]

@piotrpio Ok, then I concur with your ask to add the replicas check in checkConfig()

[kozlovic]

To be fair, this test does not prove anything since even if ConsumerReplicas() was doing nothing, this test would pass, simply because the stream itself is replicas 1, so the new consumer would inherit this value. So the test should either be moved to test/ directory where we may have some utilities to create a cluster and in that case the stream should be created with replicas 3 and consumer with 1, and in that case this test here is valid, or, before the js.Subscribe() call, create a NATS subscription on the durable create request and check that the request contains the replicas field. Check test func TestJetStreamConsumerConfigReplicasAndMemStorage() to see how I am doing this.

[goku321]

@kozlovic I totally agree on this. Will make the changes

[piotrpio]

LGTM! It would be great if you could squash your commits before we'll merge.

[kozlovic]

Check formatting (go fmt) since that failed the checks.

[kozlovic]

@goku321 As @piotrpio suggested, it would be good if you did squash all your commits into one. Could you do that please?

[goku321]

@kozlovic I merged main into my branch to resolve conflicts. Is it possible to squash all the commits now?

[kozlovic]

@goku321 Wished you had done a rebase instead of a merge, would have been easier for you to squash all the commits. I guess we can use the "Squash and Merge" in the drop-down list. Not sure, @wallyqs or @piotrpio do you know if that would work well?

[wallyqs]

sounds good to squash and merge keeping the title from the first commit via GitHub UI