Add option to set memory storage to true for a consumer #1078

goku321 · 2022-09-14T06:16:15Z

This allows forcing memory storage through sub options while subscribing.

coveralls · 2022-09-14T06:20:44Z

Coverage decreased (-0.02%) to 85.527% when pulling b64c208 on goku321:memory-option into 25b6392 on nats-io:main.

piotrpio · 2022-09-14T15:34:31Z

js.go

@@ -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 {


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.

Suggested change

if u.MemoryStorage && !s.MemoryStorage {

if u.MemoryStorage != !s.MemoryStorage {

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.

piotrpio · 2022-09-14T15:37:18Z

js_test.go

+	opts := natsserver.DefaultTestOptions
+	opts.Port = -1
+	opts.JetStream = true
+	s := natsserver.RunServer(&opts)


You can just use s := RunBasicJetStreamServer() instead.

piotrpio · 2022-09-14T15:39:38Z

js.go

@@ -2485,6 +2488,14 @@ func ConsumerReplicas(replicas int) SubOpt {
 	})
 }

+// SetMemoryStorage sets the memory storage to true for a consumer.
+func SetMemoryStorage() SubOpt {


To be more in line with other sub options, I would suggest changing the name to MemoryStorage()

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).

I named it "SetMemoryStorage" because the name "MemoryStorage" is already being used. Should I name it "ConsumerMemoryStorage" as @kozlovic suggested?

Yes please, go with ConsumerMemoryStorage() then, and this proves my point about possible name clashes unfortunately...

kozlovic · 2022-09-14T16:00:46Z

js.go

@@ -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 {


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.

kozlovic · 2022-09-14T16:03:00Z

js.go

@@ -2485,6 +2488,14 @@ func ConsumerReplicas(replicas int) SubOpt {
 	})
 }

+// SetMemoryStorage sets the memory storage to true for a consumer.
+func SetMemoryStorage() SubOpt {


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).

kozlovic · 2022-09-14T16:04:25Z

js_test.go

+		t.Fatalf("Error on subscribe: %v", err)
+	}
+
+	consInfo, err := sub.ConsumerInfo()


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.

kozlovic · 2022-09-14T16:05:50Z

js_test.go

+
+	if !consInfo.Config.MemoryStorage {
+		t.Fatalf("Expected memory storage to be %v, got %+v", true, consInfo.Config.MemoryStorage)
+	}


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.

kozlovic · 2022-09-15T17:21:11Z

@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!

goku321 · 2022-09-15T17:53:34Z

Sure @kozlovic I can change the name now and will update the tests later :)

kozlovic · 2022-09-15T18:08:31Z

@goku321 Yes, please update the PR with the option name change and tests can be updated later on. Thanks!

kozlovic

LGTM

kozlovic · 2022-09-15T18:52:43Z

@piotrpio The changes look good to me. Not sure if you want chime in or approve and merge the PR.

piotrpio

LGTM as well

Add option to set memory storage to true for a consumer

8450fa7

Add a check in checkConfig

8f89f51

derekcollison requested review from kozlovic and piotrpio September 14, 2022 14:54

piotrpio requested changes Sep 14, 2022

View reviewed changes

kozlovic requested changes Sep 14, 2022

View reviewed changes

Rename SetMemoryStorage to ConsumerMemoryStorage

b64c208

kozlovic approved these changes Sep 15, 2022

View reviewed changes

piotrpio approved these changes Sep 15, 2022

View reviewed changes

piotrpio merged commit 866ce08 into nats-io:main Sep 15, 2022

goku321 deleted the memory-option branch September 16, 2022 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to set memory storage to true for a consumer #1078

Add option to set memory storage to true for a consumer #1078

goku321 commented Sep 14, 2022

coveralls commented Sep 14, 2022 •

edited

Loading

piotrpio Sep 14, 2022

kozlovic Sep 14, 2022

piotrpio Sep 14, 2022

piotrpio Sep 14, 2022

kozlovic Sep 14, 2022

goku321 Sep 15, 2022

kozlovic Sep 15, 2022

kozlovic Sep 14, 2022

kozlovic Sep 14, 2022

kozlovic Sep 14, 2022

kozlovic Sep 14, 2022

kozlovic commented Sep 15, 2022

goku321 commented Sep 15, 2022

kozlovic commented Sep 15, 2022

kozlovic left a comment

kozlovic commented Sep 15, 2022

piotrpio left a comment

	if u.MemoryStorage && !s.MemoryStorage {
	if u.MemoryStorage != !s.MemoryStorage {

Add option to set memory storage to true for a consumer #1078

Add option to set memory storage to true for a consumer #1078

Conversation

goku321 commented Sep 14, 2022

coveralls commented Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic commented Sep 15, 2022

goku321 commented Sep 15, 2022

kozlovic commented Sep 15, 2022

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic commented Sep 15, 2022

piotrpio left a comment

Choose a reason for hiding this comment

coveralls commented Sep 14, 2022 •

edited

Loading