-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[extension/filestorage] Online file storage compaction #9327
[extension/filestorage] Online file storage compaction #9327
Conversation
56ce702
to
9a32567
Compare
I wonder if we shouldn't separate the changes adding the new compaction condition from changes to the interface. Before, |
9a32567
to
5fad271
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.
I did some integration testing of this PR. With changes proposed in this review, it worked perfectly fine in a standard k8s environment, pushing around 10k records per second through compactions as the persistent queue grew and drained.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Hi, isn't this dev going to be merged ? |
@overmeulen sorry for the delay, I'm getting back to this right now |
d7193a8
to
b8a53ad
Compare
b8a53ad
to
46e3556
Compare
0ae6a0e
to
b481e78
Compare
- `compaction.rebound_size_below_mib` (default: 10) - specifies the maximum size of actually allocated data for compaction to happen | ||
- `compaction.rebound_total_size_above_mib` (default: 100) - specifies the minimum overall size of the allocated space (both actually used and free pages) |
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.
Were these terms, "total size above" and "size below", chosen for this extension, or are they established elsewhere?
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.
This are new terms. I spent some time to figure out names that would be a good match here and this is what I came up with eventually (e.g. using "rebound" for what happens here). If you have any other suggestions, those are always welcome :)
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 think "rebound" is a very intuitive term.
I found these two parameters to be a little less intuitive, but ultimately I think between the diagram and descriptions it becomes reasonably clear.
If I were to chose alternate terms, I might try to include the word "threshold", which I think would convey that something happens when the value is crossed. I also think it might be helpful to present these in a way that more directly reflects the statefulness of the system. Maybe something like:
compaction.rebound_needed_threshold_mib
(default: 100) - when allocated data exceeds this amount, the "compaction needed" flag will be setcompaction.rebound_trigger_threshold_mib
(default: 10) - if the "compaction needed" flag is set and allocated data drops below this amount, compaction will begin and the "compaction needed" flag will be cleared
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.
That's a great advice, I like these suggestions a lot @djaglowski. What do you think @swiatekm-sumo? I am going to apply these
28a80dc
to
8500382
Compare
@pmm-sumo, looks good to me. Let me know if I should merge or wait for @swiatekm-sumo to weigh in |
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.
Some minor nits, looks good overall.
Co-authored-by: Daniel Jaglowski <[email protected]>
ade6036
to
614d1cd
Compare
Description:
Adds online filestorage compaction
Link to tracking Issue: Addresses core/#5159
Testing: Benchmarks and several tests added. Also tested manually in several scenarios
Documentation: README.md updated