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

There were situations where invalid subjects could be assigned to streams. #2721

Merged
merged 2 commits into from
Dec 1, 2021

Conversation

derekcollison
Copy link
Member

This will patch them on the fly during recovery. Specifically subjects with leading or trailing spaces and mirror streams with any subjects at all.

Signed-off-by: Derek Collison derek@nats.io

/cc @nats-io/core

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, I'd add a trace

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but would add a warning for subjects in mirrors..

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but if I recall RI's PR was checking for deliver subject. Does not seem to be covered here (would be more for consumers, not streams).

@derekcollison
Copy link
Member Author

I confirmed with R.I. that the consumer thing was not reported, he just added it. We could add a test, but he does check now so did not think it would be worth the recovery fixup since never reported.

@ripienaar
Copy link
Contributor

ripienaar commented Dec 1, 2021

A consumer in that state will never work. So users probsbly already fixed it themselves. I just wanted to have a better UX.

a stream in this situation could have one of n bad subjects and would have worked and we need to avoid the unrecoverable situation where a bad stream has data and then becomes unusable.

So we don’t need to bother with consumers imo

…eams.

This will patch them on the fly during recovery. Specifically subjects with leading or trailing spaces and mirror streams with any subjects at all.

Signed-off-by: Derek Collison <derek@nats.io>
Signed-off-by: Derek Collison <derek@nats.io>
@kozlovic kozlovic force-pushed the bad_stream_subjects branch from f03b250 to 6f5263e Compare December 1, 2021 21:01
@kozlovic
Copy link
Member

kozlovic commented Dec 1, 2021

I have rebased from main, resolved the conflicts and push forced.

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kozlovic kozlovic merged commit adf974d into main Dec 1, 2021
@kozlovic kozlovic deleted the bad_stream_subjects branch December 1, 2021 21:19
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.

4 participants