Add option to set memory storage to true for a consumer #1078
Conversation
| @@ -1357,6 +1357,9 @@ func checkConfig(s, u *ConsumerConfig) error { | |||
| if u.Replicas > 0 && u.Replicas != s.Replicas { | |||
| return makeErr("replicas", u.Replicas, s.Replicas) | |||
| } | |||
| if u.MemoryStorage && !s.MemoryStorage { | |||
There was a problem hiding this comment.
I think we should be verifying it both ways, i.e. true -> false and false -> true. Right now check will pass even if u.MemoryStorage == false and s.MemoryStorage == true.
| if u.MemoryStorage && !s.MemoryStorage { | |
| if u.MemoryStorage != !s.MemoryStorage { |
There was a problem hiding this comment.
No we can't unfortunately, because we don't have a "3 states" value: true, false, not-specified. The point of this function is to check the user intent vs what is already configured in the server.
We have no way to express that the user does not want MemoryStorage, so if you call js.Subscribe() without specifying anything and the JS consumer happens to be configured on the server with MemoryStorage, you don't want to call to fail. Otherwise we would need an option that says "NoMemoryStorage", etc..
So this function for boolean is not great, but the best we can do is check when the intent was to set a boolean to true, then only we can check that it matches the server setting.
| opts := natsserver.DefaultTestOptions | ||
| opts.Port = -1 | ||
| opts.JetStream = true | ||
| s := natsserver.RunServer(&opts) |
There was a problem hiding this comment.
You can just use s := RunBasicJetStreamServer() instead.
js.go
Outdated
| @@ -2485,6 +2488,14 @@ func ConsumerReplicas(replicas int) SubOpt { | |||
| }) | |||
| } | |||
|
|
|||
| // SetMemoryStorage sets the memory storage to true for a consumer. | |||
| func SetMemoryStorage() SubOpt { | |||
There was a problem hiding this comment.
To be more in line with other sub options, I would suggest changing the name to MemoryStorage()
There was a problem hiding this comment.
Yes. Also wonder when the name is "too generic", is going to be a problem if one day we need the same name for a different option? Meaning this is a SubOpt, but what if we need an option one day for JSOpt (or any other). For that reason, should we be more precise in the naming? Something like "ConsumerMemoryStorage()"? (not good, but you get the idea).
There was a problem hiding this comment.
I named it "SetMemoryStorage" because the name "MemoryStorage" is already being used. Should I name it "ConsumerMemoryStorage" as @kozlovic suggested?
There was a problem hiding this comment.
Yes please, go with ConsumerMemoryStorage() then, and this proves my point about possible name clashes unfortunately...
| @@ -1357,6 +1357,9 @@ func checkConfig(s, u *ConsumerConfig) error { | |||
| if u.Replicas > 0 && u.Replicas != s.Replicas { | |||
| return makeErr("replicas", u.Replicas, s.Replicas) | |||
| } | |||
| if u.MemoryStorage && !s.MemoryStorage { | |||
There was a problem hiding this comment.
No we can't unfortunately, because we don't have a "3 states" value: true, false, not-specified. The point of this function is to check the user intent vs what is already configured in the server.
We have no way to express that the user does not want MemoryStorage, so if you call js.Subscribe() without specifying anything and the JS consumer happens to be configured on the server with MemoryStorage, you don't want to call to fail. Otherwise we would need an option that says "NoMemoryStorage", etc..
So this function for boolean is not great, but the best we can do is check when the intent was to set a boolean to true, then only we can check that it matches the server setting.
js.go
Outdated
| @@ -2485,6 +2488,14 @@ func ConsumerReplicas(replicas int) SubOpt { | |||
| }) | |||
| } | |||
|
|
|||
| // SetMemoryStorage sets the memory storage to true for a consumer. | |||
| func SetMemoryStorage() SubOpt { | |||
There was a problem hiding this comment.
Yes. Also wonder when the name is "too generic", is going to be a problem if one day we need the same name for a different option? Meaning this is a SubOpt, but what if we need an option one day for JSOpt (or any other). For that reason, should we be more precise in the naming? Something like "ConsumerMemoryStorage()"? (not good, but you get the idea).
| t.Fatalf("Error on subscribe: %v", err) | ||
| } | ||
|
|
||
| consInfo, err := sub.ConsumerInfo() |
There was a problem hiding this comment.
Instead of repeating this code for the 3 types of consumers, I would have made it a small helper function inline:
checkCons := func(sub *Subscription) {
t.Helper()
consInfo, err := sub.ConsumerInfo()
...
}
then you would invoke it after each sub creation.
|
|
||
| if !consInfo.Config.MemoryStorage { | ||
| t.Fatalf("Expected memory storage to be %v, got %+v", true, consInfo.Config.MemoryStorage) | ||
| } |
There was a problem hiding this comment.
What you may want to add is a test where a durable is created with MemoryStorage and call a js.SubscribeSync() with Durable() but without the memory storage option in the subscribe call and verifies that it actually works. That would catch the change the Piotr was suggesting that would - in my opinion - break things.
|
@goku321 Do you think you could get the changes in soon? We are trying to release today or tomorrow (Friday) and since this is an addition it would be better to go in the 1.17.0 than in a 1.17.1 :-). The test could be changed/updated later, however it would be nice to change the name of the option now as @piotrpio suggested. Thanks! |
|
Sure @kozlovic I can change the name now and will update the tests later :) |
|
@goku321 Yes, please update the PR with the option name change and tests can be updated later on. Thanks! |
|
@piotrpio The changes look good to me. Not sure if you want chime in or approve and merge the PR. |
This allows forcing memory storage through sub options while subscribing.