-
Notifications
You must be signed in to change notification settings - Fork 463
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
feat(pageserver): support retain_lsn in bottommost gc-compaction #8328
Conversation
3126 tests run: 3005 passed, 0 failed, 121 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
24fb4d5 at 2024-07-23T19:24:25.817Z :recycle: |
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.
Did a first review pass.
added tests for the core |
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Co-authored-by: Christian Schwarz <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
7cb0f35
to
31b6c2e
Compare
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
added timeline e2e unit tests |
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.
So, apart from the added tests, changes since my last review are that we now use lowest_retain_lsn
instead of gc_cutoff
. What was wrong there?
Also, are you sure that always passing lowest_retain_lsn is correct?
Suppose you have the case of a lot of delta for a given key range. So much delta that we need to create multiple deltas with same key range but split up LSN ranges. The current code, afaict, will create multiple deltas with LSN_range.start == lowest_retain_lsn. No?
Possibly this is a pre-existing issue though?
The lowest image layer is created over lowest_retain_lsn instead of gc_cutoff. gc_cutoff can be considered as a retain_lsn.
Yes, for now, the bottom-most compaction always produce delta layers of the LSN range from lowest_retain_lsn to the GC horizon. We could further split it over the LSN as an optimization in the future. |
Co-authored-by: Christian Schwarz <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Are we not respecting the target file size yet? |
Yes, not for now, this is tracked in the epic. |
…h/bottommost-retain-lsn
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
Signed-off-by: Alex Chi Z <[email protected]>
6f9685b
to
24fb4d5
Compare
should be working after #8328 gets merged. Part of #8002 adds a new perf benchmark case that ensures garbages can be collected with branches --------- Signed-off-by: Alex Chi Z <[email protected]>
should be working after #8328 gets merged. Part of #8002 adds a new perf benchmark case that ensures garbages can be collected with branches --------- Signed-off-by: Alex Chi Z <[email protected]>
Problem
part of #8002
Summary of changes
The main thing in this pull request is the new
generate_key_retention
function. It decides which deltas to retain and generate images for a given key based on its history + retain_lsn + horizon.On that, we generate a flat single level of delta layers over all deltas included in the compaction. In the future, we can decide whether to split them over the LSN axis as described in the RFC.
Checklist before requesting a review
Checklist before merging