Allow setting consumer replicas though options #1019
Conversation
|
Hey, this looks good, I just have 2 suggestions:
|
| // ConsumerReplicas sets the number of replica count for a consumer. | ||
| func ConsumerReplicas(replicas int) SubOpt { | ||
| return subOptFn(func(opts *subOpts) error { | ||
| if replicas < 1 { |
There was a problem hiding this comment.
I think we can also consider setting adding && replicas <=5, as that is the upper limit.
There was a problem hiding this comment.
That is the current server limit, but since server defined and we could change would leave that out..
|
@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? |
|
@kozlovic Simplified JestStream API will be in it's own package, so the current implementation can be modified separately. |
js_test.go
Outdated
| } | ||
|
|
||
| // Check if the number of replicas is the same as we provided. | ||
| if consInfo.Config.Replicas != 1 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@kozlovic I totally agree on this. Will make the changes
|
@kozlovic I merged main into my branch to resolve conflicts. Is it possible to squash all the commits now? |
|
sounds good to squash and merge keeping the title from the first commit via GitHub UI |
Currently, only
js.AddConsumersupports configuring replicas count for a consumer. Configuring it throughnats.SubOptwill allow configuring the same withjs.Subscribe,js.PullSubscribeetc.