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

Fix stream snapshot error messaging #573

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

MauriceVanVeen
Copy link
Member

#545 introduced checking if during a stream snapshot there was some sort of error.

However Nats-Request-Info was checked which doesn't seem to contain this info.
The server reports the header NATS/1.0 408 No Interest which is reflected in the headers as:
{Status: "408", Description: "No Interest"}.

Source for reference: https://github.com/nats-io/nats-server/blob/a07bde9fa7d499c7039f74cac9d11f4c046b9250/server/jetstream_api.go#L3826

This PR fixes using the correct header, and also only calling cancel() after we've added the error to the errc as otherwise the error would not be logged.

Signed-off-by: Maurice van Veen <[email protected]>
@MauriceVanVeen MauriceVanVeen force-pushed the fix/stream-snapshot-error-messaging branch from ff201f1 to 7a8d8e3 Compare September 10, 2024 16:18
@MauriceVanVeen MauriceVanVeen marked this pull request as ready for review September 10, 2024 16:21
@ripienaar
Copy link
Collaborator

@MauriceVanVeen hmm, I totally forgot about this one, is it still good to merge?

@MauriceVanVeen
Copy link
Member Author

@ripienaar, yep good to merge, but it's awaiting the server PR: nats-io/nats-server#5873

@MauriceVanVeen
Copy link
Member Author

@ripienaar, the server PR has been merged, this one is now also good to go 👍

@ripienaar ripienaar merged commit 2a95464 into main Dec 17, 2024
6 checks passed
@ripienaar
Copy link
Collaborator

Thank you @MauriceVanVeen

@ripienaar ripienaar deleted the fix/stream-snapshot-error-messaging branch December 17, 2024 14:59
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.

2 participants