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
Don't hold entire MQTT retained messages in memory #4228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could change signature of one function since we don't seem to need the old sequence anymore. But more importantly, I wonder if the mqttCheckPubRetainedPerms() function should not be refactored a little bit knowing that we now have to read messages (possibly from disk, and possibly a lot of them).
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
…'t hold locks longer than needed Signed-off-by: Neil Twigg <neil@nats.io>
f7380de
to
3b07f43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will review more later, I can't at the moment but just wanted to post that note about asm.sl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could replace a write lock with a read lock, but other than that LGTM.
server/mqtt.go
Outdated
// Get all of the retained messages. Then we will sort them so | ||
// that they are in sequence order, which should help the file | ||
// store to not have to load out-of-order blocks so often. | ||
asm.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use RLock()/RUnlock() here since mqttAccountSessionManager.mu
is a sync.RWMutex
and you are just reading from asm.retmsgs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Thanks for your reviews on this!
Signed-off-by: Neil Twigg <neil@nats.io>
9fe220f
to
afe7f48
Compare
This PR separates out the small amount of necessary metadata for retained messages (stream sequence, floor) from the message itself, instead accessing the messages themselves with KV-like access patterns.
This should save quite a bit of memory where there are lots of retained messages since we only now need to hold a small amount of metadata instead of the entire messages.
Signed-off-by: Neil Twigg neil@nats.io