[VQueues] Increase successive-merges limit and remove wasteful partial merges#4747
[VQueues] Increase successive-merges limit and remove wasteful partial merges#4747AhmedSoliman wants to merge 1 commit into
Conversation
AhmedSoliman
force-pushed
the
pr4747
branch
4 times, most recently
from
fe0d1b2 to
50391e4
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50391e4aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…l merges Partial merges didn't add value and wasted precious CPU cycles since the updates cannot be merged This change removes it and increase the limit of successive merges from 100 (way too low for vqueus) to 5000100 (way too low for vqueus) to 5000 which experimentally seems to be enough, but we may expose this as an advanced config option in the future if really needed.
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for creating this PR @AhmedSoliman. LGTM. +1 for merging.
The one question I had was why is it a good idea to make merges happening less likely when being stored in the memtable (increasing the max successive merges value)?
| KeyKind::partial_merge, | ||
| ); | ||
| cf_options.set_max_successive_merges(100); | ||
| cf_options.set_max_successive_merges(5000); |
There was a problem hiding this comment.
Increasing this value will make it less likely that merges are happening in the memtable, right? Is this something to prevent writes from becoming more expensive and delay the merges to happen during compactions?
2 participants
Partial merges didn't add value and wasted precious CPU cycles since the updates cannot be merged
This change removes it and increase the limit of successive merges from 100 (way too low for vqueus) to 5000100 (way too low for vqueus) to 5000
which experimentally seems to be enough, but we may expose this as an advanced config option in the future if really needed.
Stack created with Sapling. Best reviewed with ReviewStack.