feat(pageserver): support partial gc-compaction for delta layers #9611
Conversation
No tests were run or test report is not availableTest coverage report is not availableThe comment gets automatically updated with the latest test results
647fe84 at 2024-11-07T16:46:55.267Z :recycle: |
There was a problem hiding this comment.
The final patch for partial compaction,
Hm, is this truly the final piece of work in partial bottommost compaction? We don't support partial bottommost compaction yet.
We should not have a not-partial-bottommost-compact going forward, to reduce the amount of potential code paths that need to be kept in mind / reviewed.
Hence, can we please switch the API to take the compaction_key_range a Range<Key> instead of Option<Range<Key>>.
Full bottommost compaction unit tests can pass Key::MIN..Key::MAX. Or should it be just .. ? Or Key::Min..=Key::MAX.
Anyway, hope you get the point.
| if let Some(err) = check_valid_layermap(&layer_names) { | ||
| bail!("cannot run gc-compaction because {}", err); | ||
| warn!("gc-compaction layer map check failed because {}, this is normal if partial compaction is not finished yet", err); | ||
| } |
There was a problem hiding this comment.
I thought a week or two ago you said that you wanted to relax the definition of what a valid layer map is towards "as long as no layers overlap it's valid".
There was a problem hiding this comment.
Yes, but doing that check would be O(n^2) and I don't want to implement it that way, so let's warn here first.
| false | ||
| }; | ||
| let last_key: &mut Key = last_key.as_mut().unwrap(); | ||
| stat.on_unique_key_visited(); // TODO: adjust statistics for partial compaction |
There was a problem hiding this comment.
Tracking this in a follow-up task already? I'm not sure I understand what needs to be done.
There was a problem hiding this comment.
It depends on whether we want to include keys not in the partial compaction range into the statistics. Currently I use the statistics to know how much space is saved by running the compaction.
| if let Some(compaction_key_range) = &job_desc.partial_key_range { | ||
| for layer in job_desc.rewrite_layers { |
There was a problem hiding this comment.
Would be nice to encode in the type system that rewrite_layers is only meaningful if partial_key_range is some.
Maybe by using a custom enum instead of Option for partial_key_range.
Eventually, partial compaction should be the only thing we do (not in this PR, branches are not yet supported)
| // This download should be no-op b/c it's included in selected_layer and has been downloaded before | ||
| let resident_layer = layer.download_and_keep_resident().await?; |
There was a problem hiding this comment.
Why not store ResidentLayers in rewrite_layers and selected_layers then?
skyzh
force-pushed
the
skyzh/partial-compaction-delta
branch
from
24cd910
to
4896c0b
Compare
Signed-off-by: Alex Chi Z <[email protected]> partial address comments Signed-off-by: Alex Chi Z <[email protected]>
…c-compaction (#9671) Signed-off-by: Alex Chi Z <[email protected]>
skyzh
force-pushed
the
skyzh/partial-compaction-delta
branch
from
16cc56f
to
647fe84
Compare
Problem
The final patch for partial compaction, part of #9114, close #8921 (note that we didn't implement parallel compaction or compaction scheduler for partial compaction -- currently this needs to be scheduled by using a Python script to split the keyspace, and in the future, automatically split based on the key partitioning when the pageserver wants to trigger a gc-compaction)
Summary of changes
Checklist before requesting a review
Checklist before merging