-
Notifications
You must be signed in to change notification settings - Fork 689
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
Change expected last sequence pub options to pointers #958
Conversation
This allows for differentiating between the zero value and whether it was intentionally set to a sequence of zero. Signed-off-by: Byron Ruth <b@devel.io>
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.
LGTM..
if o.seq > 0 { | ||
m.Header.Set(ExpectedLastSeqHdr, strconv.FormatUint(o.seq, 10)) | ||
if o.seq != nil { | ||
m.Header.Set(ExpectedLastSeqHdr, strconv.FormatUint(*o.seq, 10)) |
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 was going to say that it does not matter since '0' is not used by the server, which is true for that header (https://github.com/nats-io/nats-server/blob/0df5da3924799cfab51151c1143421fca3bcb4c8/server/stream.go#L2957) but not for the other indeed (https://github.com/nats-io/nats-server/blob/0df5da3924799cfab51151c1143421fca3bcb4c8/server/stream.go#L2996).
So I think that we could accept the change, and this should not break existing apps. Let's see if @wallyqs has a comment.
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.
Would also be nice if the server had it on both for consistency?
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.
@kozlovic Thanks for pointing that out. FWIW, I agree with @ripienaar that it should be the same behavior for both subject and stream-level.
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.
So I created a PR for the server but changed it to Draft. As Derek mentioned, this would work only for the very first message since after that, the stream first sequence should never go back to 0. There may still be value, but being close to 2.8.0 release, although we could add it as a "CHANGED" entry in the release notes, maybe we will wait for the 2.9.0 release as RI suggested.
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.
@kozlovic Thanks and understood. I presume we can still move forward with this PR so the subject-level header will have its behavior corrected?
We use to ignore if the seq was 0, but now would treat it as a requirement that the stream be empty if header is present but set to 0. This relates to client PR: nats-io/nats.go#958 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@@ -822,15 +822,15 @@ func ExpectStream(stream string) PubOpt { | |||
// ExpectLastSequence sets the expected sequence in the response from the publish. | |||
func ExpectLastSequence(seq uint64) PubOpt { | |||
return pubOptFn(func(opts *pubOpts) error { | |||
opts.seq = seq | |||
opts.seq = &seq |
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.
Maybe add a test like this to cover the behavior change, header is persisted now along with the message but should be fine.
func TestJetStreamPublishExpectZero(t *testing.T) {
s := RunBasicJetStreamServer()
defer shutdownJSServerAndRemoveStorage(t, s)
nc, js := jsClient(t, s)
defer nc.Close()
var err error
// Create the stream using our client API.
_, err = js.AddStream(&nats.StreamConfig{
Name: "TEST",
Subjects: []string{"test", "foo", "bar"},
})
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
sub, err := nc.SubscribeSync("foo")
if err != nil {
t.Errorf("Error: %s", err)
}
// Explicitly set the header to zero.
_, err = js.Publish("foo", []byte("bar"), nats.ExpectLastSequence(0))
if err != nil {
t.Errorf("Error: %v", err)
}
rawMsg, err := js.GetMsg("TEST", 1)
if err != nil {
t.Fatalf("Error: %s", err)
}
hdr, ok := rawMsg.Header["Nats-Expected-Last-Sequence"]
if !ok {
t.Fatal("Missing header")
}
got := hdr[0]
expected := "0"
if got != expected {
t.Fatalf("Expected %v, got: %v", expected, got)
}
msg, err := sub.NextMsg(1*time.Second)
if err != nil {
t.Fatalf("Error: %s", err)
}
hdr, ok = msg.Header["Nats-Expected-Last-Sequence"]
if !ok {
t.Fatal("Missing header")
}
got = hdr[0]
expected = "0"
if got != expected {
t.Fatalf("Expected %v, got: %v", expected, got)
}
}
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.
@wallyqs Added a test to check both headers, thanks for stubbing this out.
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.
LGTM!
Signed-off-by: Byron Ruth <b@devel.io>
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.
LGTM
We use to ignore if the seq was 0, but now would treat it as a requirement that the stream be empty if header is present but set to 0. This relates to client PR: nats-io/nats.go#958 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
We use to ignore if the seq was 0, but now would treat it as a requirement that the stream be empty if header is present but set to 0. This relates to client PR: nats-io/nats.go#958 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This allows for differentiating between the zero value and whether it was intentionally
set to a sequence of zero.
Signed-off-by: Byron Ruth b@devel.io