-
Notifications
You must be signed in to change notification settings - Fork 712
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
Extract ErrConsumerDeleted in Fetch() #1125
Conversation
6f09dc6
to
fd29cc5
Compare
fd29cc5
to
fc44eec
Compare
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!
Co-authored-by: Waldemar Quevedo <[email protected]>
@@ -2575,6 +2575,12 @@ func checkMsg(msg *Msg, checkSts, isNoWait bool) (usrMsg bool, err error) { | |||
// one message when making requests without no_wait. | |||
err = ErrTimeout | |||
} | |||
case jetStream409Sts: | |||
if strings.Contains(strings.ToLower(string(msg.Header.Get(descrHdr))), "consumer deleted") { |
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 how are we handling this client facing breaking change?
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.
This status was introduced last week in the server (notifying about consumer being deleted) and we only depend on "consumer deleted" string, so think we can treat this as a new feature: nats-io/nats-server#3605
WDYT?
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.
There's also a nats.go release that went around the same time, this error could have been surfaced to real users by now right?
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 meantion the nats.go release being 3 days ago to highlight that the next nats.go release is probably not tomorrow - meaning the delta between current users on a current server and this change langing in a release is likely to be quite big, relatively speaking. Thus increasing the risk that this will impact real users.
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.
You're right about that. To shorten the delta we could issue a patch release sooner (e.g. tomorrow), minimizing the impact.
Resolves #1121