-
Notifications
You must be signed in to change notification settings - Fork 0
feat(persistence): automatic AOF rewrite + un-gated multi-shard BGREWRITEAOF (#433) #443
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not run a filesystem walk on every
INFOcall.Line 288 calls
refresh_current_size(), which callsmeasure_total_size()insrc/persistence/aof/auto_rewrite.rs. That function walks the wholeappendonlydirtree recursively and issues onemetadatasyscall per file, plus one for the legacy flat file.The comment says INFO is a cold path. Two facts contradict that:
infoat Line 172 ignores_argsand always builds every section. A bareINFOtherefore triggers the walk, not onlyINFO persistence.INFOcontinuously, often from several collectors at once.The walk is synchronous blocking IO on the shard event loop thread that serves the connection, so it stalls command processing for that shard. The cost grows with the number of retained generations and shards.
The auto-rewrite monitor already stores a fresh value in
AOF_CURRENT_SIZEevery second. Read that atomic instead and accept at most one second of staleness.init()seeds the atomic throughrecord_base_size(), so it is populated before the firstINFO.⚡ Proposed fix
let aof_enabled = crate::persistence::aof::auto_rewrite::AOF_ENABLED .load(std::sync::atomic::Ordering::Relaxed); let aof_current_size = if aof_enabled { - crate::persistence::aof::auto_rewrite::refresh_current_size() + // Read the monitor's cached sample (refreshed every TICK). A directory + // walk here would run on the shard event loop on every INFO call. + crate::persistence::aof::auto_rewrite::AOF_CURRENT_SIZE + .load(std::sync::atomic::Ordering::Relaxed) } else { 0 };📝 Committable suggestion
🤖 Prompt for AI Agents