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.

Sign up for GitHub

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

Jump to bottom

Per-subject limits for MQTT retained messages #4199

Merged
merged 4 commits into from Jun 1, 2023
Merged

Per-subject limits for MQTT retained messages #4199

merged 4 commits into from Jun 1, 2023

Conversation

neilalexander
Copy link
Member

By publishing retained messages on their own subject, we can apply max messages per subject limits to them. Also include the domain name in the subject to make consistent with the other MQTT streams.

Signed-off-by: Neil Twigg neil@nats.io

@neilalexander
Copy link
Member Author

Still need to handle migrating the old streams to the new subjects before review is needed.

@neilalexander neilalexander marked this pull request as ready for review May 31, 2023 11:38
@neilalexander neilalexander requested a review from a team as a code owner May 31, 2023 11:38
@kozlovic
Copy link
Member

kozlovic commented Jun 1, 2023

@neilalexander I made some changes because running a manual test I noticed that messages would not be received after the transfer. See comments on my commit.
As a general rule and experience from the client library, updating the stream is something in the code is risky because moving from older/newer/older versions could "erase" some new settings on the stream since an older version of a server would have a stream config that does not contain fields from a newer release. I am not sure if it is an issue here, but again, something to keep in mind. Maybe in those cases (where we need to update configs), we may want to make that external - as a tool or part of nats CLI).

kozlovic
kozlovic approved these changes Jun 1, 2023
View reviewed changes
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, change was good but there was an issue with not refreshing the stream info after transfer. I am posting an LGTM here for your code, but please review my changes too.

@neilalexander
Copy link
Member Author

@kozlovic Thanks for the review and the fixes! I've taken a look over your commit and it makes sense and looks fine to me — I had missed the need to call lookupStream again in order to keep the stream state up-to-date after the migration. Will rebase and merge.

neilalexander and others added 4 commits June 1, 2023 10:00
@neilalexander
Per-subject limits for MQTT retained messages
7469038 
Signed-off-by: Neil Twigg <neil@nats.io>
@neilalexander
Migrate old retained messages to new subjects
007565f 
Signed-off-by: Neil Twigg <neil@nats.io>
@neilalexander
Add test for MQTT retained message migration
4f797a5 
Signed-off-by: Neil Twigg <neil@nats.io>
@kozlovic @neilalexander
Fixed delivery of retained messages after transfer.
a744cb8 
I was running a manual test moving from dev to this branch and
noticed that the consumer would receive only 1 message of the 10
messages sent as retained. So I modified the test to verify that
we receive them all and we did not.

The reason was that after the transfer we need to refresh the state
of the stream (stream info) since we attempt to load all messages
based on the state's sequences.

I have also modified a bit the code to update the MaxMsgsPer once
all messages have been transferred.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@neilalexander neilalexander merged commit 30fa427 into dev Jun 1, 2023
2 checks passed
@neilalexander neilalexander deleted the neil/mqttretained branch June 1, 2023 09:22
@kozlovic kozlovic mentioned this pull request Aug 10, 2023
Merged
3 tasks
@levb levb mentioned this pull request Nov 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlovic kozlovic kozlovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@neilalexander @kozlovic