New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to set memory storage to true for a consumer #1078
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
return makeErr("memory storage", u.MemoryStorage, s.MemoryStorage) | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be more in line with other sub options, I would suggest changing the name to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes please, go with ConsumerMemoryStorage() then, and this proves my point about possible name clashes unfortunately... |
||
return subOptFn(func(opts *subOpts) error { | ||
opts.cfg.MemoryStorage = true | ||
return nil | ||
}) | ||
} | ||
|
||
func (sub *Subscription) ConsumerInfo() (*ConsumerInfo, error) { | ||
sub.mu.Lock() | ||
// TODO(dlc) - Better way to mark especially if we attach. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1096,3 +1096,64 @@ func TestJetStreamConvertDirectMsgResponseToMsg(t *testing.T) { | |
t.Fatalf("Wrong header: %v", r.Header) | ||
} | ||
} | ||
|
||
func TestJetStreamSetMemoryStorage(t *testing.T) { | ||
opts := natsserver.DefaultTestOptions | ||
opts.Port = -1 | ||
opts.JetStream = true | ||
s := natsserver.RunServer(&opts) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just use |
||
defer shutdownJSServerAndRemoveStorage(t, s) | ||
|
||
nc, js := jsClient(t, s) | ||
defer nc.Close() | ||
|
||
if _, err := js.AddStream(&StreamConfig{Name: "STR", Subjects: []string{"foo"}}); err != nil { | ||
t.Fatalf("Error adding stream: %v", err) | ||
} | ||
|
||
// Pull ephemeral consumer with memory storage. | ||
sub, err := js.PullSubscribe("foo", "", SetMemoryStorage()) | ||
if err != nil { | ||
t.Fatalf("Error on subscribe: %v", err) | ||
} | ||
|
||
consInfo, err := sub.ConsumerInfo() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of repeating this code for the 3 types of consumers, I would have made it a small helper function inline:
then you would invoke it after each sub creation. |
||
if err != nil { | ||
t.Fatalf("Error getting consumer info: %v", err) | ||
} | ||
|
||
if !consInfo.Config.MemoryStorage { | ||
t.Fatalf("Expected memory storage to be %v, got %+v", true, consInfo.Config.MemoryStorage) | ||
} | ||
|
||
// Create a sync subscription with an in-memory ephemeral consumer. | ||
sub, err = js.SubscribeSync("foo", SetMemoryStorage()) | ||
if err != nil { | ||
t.Fatalf("Error on subscribe: %v", err) | ||
} | ||
|
||
consInfo, err = sub.ConsumerInfo() | ||
if err != nil { | ||
t.Fatalf("Error getting consumer info: %v", err) | ||
} | ||
|
||
if !consInfo.Config.MemoryStorage { | ||
t.Fatalf("Expected memory storage to be %v, got %+v", true, consInfo.Config.MemoryStorage) | ||
} | ||
|
||
// Async subscription with an in-memory ephemeral consumer. | ||
cb := func(msg *Msg) {} | ||
sub, err = js.Subscribe("foo", cb, SetMemoryStorage()) | ||
if err != nil { | ||
t.Fatalf("Error on subscribe: %v", err) | ||
} | ||
|
||
consInfo, err = sub.ConsumerInfo() | ||
if err != nil { | ||
t.Fatalf("Error getting consumer info: %v", err) | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be verifying it both ways, i.e.
true -> false
andfalse -> true
. Right now check will pass even ifu.MemoryStorage == false
ands.MemoryStorage == true
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.