Navigation Menu

Skip to content
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

Merged
merged 2 commits into from Nov 8, 2022
Merged

Conversation

piotrpio
Copy link
Collaborator

@piotrpio piotrpio commented Nov 8, 2022

Resolves #1121

@piotrpio piotrpio requested a review from wallyqs November 8, 2022 12:01
@piotrpio piotrpio force-pushed the extract-consumernotfound-err branch 2 times, most recently from 6f09dc6 to fd29cc5 Compare November 8, 2022 12:12
@coveralls
Copy link

coveralls commented Nov 8, 2022

Coverage Status

Coverage increased (+0.07%) to 85.798% when pulling cdf4cb2 on extract-consumernotfound-err into 980f955 on main.

test/js_test.go Outdated Show resolved Hide resolved
Copy link
Member

@wallyqs wallyqs left a 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 <wally@synadia.com>
@piotrpio piotrpio merged commit 4d7d951 into main Nov 8, 2022
@piotrpio piotrpio deleted the extract-consumernotfound-err branch November 8, 2022 15:22
@@ -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") {
Copy link
Contributor

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@piotrpio piotrpio mentioned this pull request Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors returned from server should be lowered cased if passed to application
4 participants