-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ancient pack: add low water mark #33785
Conversation
fb35ac2
to
ccbf033
Compare
Codecov Report
@@ Coverage Diff @@
## master #33785 +/- ##
=======================================
Coverage 81.8% 81.8%
=======================================
Files 806 806
Lines 217428 217438 +10
=======================================
+ Hits 178056 178075 +19
+ Misses 39372 39363 -9 |
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.
lgtm
@@ -182,15 +182,17 @@ impl AncientSlotInfos { | |||
self.shrink_indexes.clear(); | |||
let total_storages = self.all_infos.len(); | |||
let mut cumulative_bytes = 0u64; | |||
let low_threshold = max_storages * 50 / 100; |
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.
Is the current impl preferred over a divide by two?
let low_threshold = max_storages * 50 / 100; | |
let low_threshold = max_storages / 2; |
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.
Perhaps, introduce a const variable for this?
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.
I was thinking this is more tunable than /2. 80%, 20%, etc. Ultimately we might make this a tuning parameter? Even a cli argument? This choice seems sufficient for the moment based on mnb monitoring. And it beats the current 100 slots at a time impl.
Problem
When we disable rewrites, we will accumulate old append vecs & slots. We use ancient append vec packing to combine those.
It is more efficient to pack a larger range of slots at a time instead of a small amount each call.
Summary of Changes
Add a low water mark. By default, when there are more than 10k ancient slots, the algorithm will pack down to 5k ancient slots. Then, the algorithm will do nothing until 10k is exceeded again.
Fixes #