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

[CHANGED] MQTT RMS consumer: instead of deleting/creating, create a u… #5017

Merged
merged 3 commits into from Jan 31, 2024

Conversation

levb
Copy link
Contributor

@levb levb commented Jan 30, 2024

Every time MQTT was initialized for an account on a server, it was re-creating the server-specific RMS consumer by deleting it and creating again, with the same name. @derek though it might be causing issues and suggested this change.

Now, always create a new consumer with a unique name, and a 5 minute inactivity threshold. Kept the code to delete the "legacy"-named consumer, even though it'd get executed at most once, errors ignored.

@levb levb requested a review from kozlovic January 30, 2024 20:42
server/mqtt.go Outdated Show resolved Hide resolved
server/mqtt_test.go Outdated Show resolved Hide resolved
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.

Change look ok to me, but not sure what the issue was and how is this going to be different.

@derekcollison
Copy link
Member

derekcollison commented Jan 31, 2024

I am concerned by what I saw for a customer that might be triggered by quick succession of delete and add with same name but different NRG assignment.

@levb levb marked this pull request as ready for review January 31, 2024 13:05
@levb levb requested a review from a team as a code owner January 31, 2024 13:05
@levb
Copy link
Contributor Author

levb commented Jan 31, 2024

@derekcollison I can't get past the build error after several retries,
image
Can't see how this failure may possibly be related to my change, and this branch is up to date with main. Not sure what's going on. Will retry again.

@kozlovic
Copy link
Member

I have seen this test starting to flap on Travis. I can have a look say tomorrow, but for now, if it is a blocker, add a t.Skip() to skip this test.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM - Small comment but not blocking.

// The old one should expire and get deleted due to inactivity. The name for
// the durable is $MQTT_rmsgs_{uuid}_{server-name}, the server name is just
// for readability.
rmDurName := mqttRetainedMsgsStreamName + "_" + nuid.Next() + "_" + s.String()
Copy link
Member

Choose a reason for hiding this comment

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

Not really durable, just the name, and we only really expect it to live for the lifetime of the running server.

@derekcollison derekcollison merged commit b04d088 into main Jan 31, 2024
4 checks passed
@derekcollison derekcollison deleted the lev-mqtt-rms-consumer-nodelete branch January 31, 2024 16:16
wallyqs added a commit that referenced this pull request Feb 1, 2024
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.

None yet

3 participants