-
Notifications
You must be signed in to change notification settings - Fork 396
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
[DNM/PoC]: use lsn leases to temporarily block gc #7996
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
db378d1 at 2024-06-17T19:19:33.110Z :recycle: |
0176cc0
to
4e99dd7
Compare
// Gets the maximum LSN that holds the valid lease. | ||
// Caveat: This value could be stale since we rely on refresh_gc_info to invalidate leases, | ||
// so there could be leases invalidated between the refresh and here. | ||
let max_lsn_with_valid_lease = gc_info.leases.last_key_value().map(|(lsn, _)| *lsn); |
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.
Current implementation would keep everything above the lease. Would be better if this is handled in the same way as retain_lsn so that we only keep necessary images? I forgot where our discussion last time ends up 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.
I'm actually trying to handle it the same way as what retain_lsns
does right now. The way current GC uses retain_lsns
is that for each layer it loops through the retain_lsn
in retain_lsns
, and keeping the layer if layer.get_lsn_range().start <= retain_lsn
.
In my case, I'm doing layer.get_lsn_range().start <= maximum_lsn_with_valid_lease
(If a layer's lsn_range
overlaps with the maximum_lsn_with_valid_lease
, then all other LSNs with valid leases will do as well).
neon/pageserver/src/tenant/timeline.rs
Lines 5051 to 5083 in 4f13e03
// 3. Is it needed by a child branch? | |
// NOTE With that we would keep data that | |
// might be referenced by child branches forever. | |
// We can track this in child timeline GC and delete parent layers when | |
// they are no longer needed. This might be complicated with long inheritance chains. | |
// | |
// TODO Vec is not a great choice for `retain_lsns` | |
for retain_lsn in &retain_lsns { | |
// start_lsn is inclusive | |
if &l.get_lsn_range().start <= retain_lsn { | |
debug!( | |
"keeping {} because it's still might be referenced by child branch forked at {} is_dropped: xx is_incremental: {}", | |
l.layer_name(), | |
retain_lsn, | |
l.is_incremental(), | |
); | |
result.layers_needed_by_branches += 1; | |
continue 'outer; | |
} | |
} | |
// 3.5 Is there a valid lease that requires us to keep this layer? | |
if let Some(lsn) = &max_lsn_with_valid_lease { | |
if &l.get_lsn_range().start <= lsn { | |
debug!( | |
"keeping {} because there is a valid lease preventing GC at {}", | |
l.layer_name(), | |
lsn, | |
); | |
result.layers_needed_by_leases += 1; | |
continue 'outer; | |
} | |
} |
Maybe I miss the part where retain_lsns
only keep necessary images? One way we could do that is to maybe check the end of the lsn range as well?
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.
Okay seems like retain_lsn did not optimize for branches (it keeps everything above the oldest branch). Would be a future optimization.
4f13e03
to
73fee3c
Compare
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
e31a896
to
a5c8f94
Compare
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
ca3f0d4
to
be8f200
Compare
pageserver/src/tenant/timeline.rs
Outdated
if let Some(lsn) = &min_lsn_with_valid_lease { | ||
if &l.get_lsn_range().start <= lsn { |
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.
layer.get_lsn_range().start
<= min_lsn_with_valid_lease
<= all other lsn with valid leases.
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.
The comparison should actually be against max_lsn_with_valid_lease
since we keep a layer if the layer start is <= any of the lease.
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.
Looking good, few smaller comments.
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
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. Could have some e2e test once the compute side of this pull request gets merged. Besides the "one compute should hold one lease" problem I mentioned in the review comments, I also prefer that at some point in the future gc_timeline
should take all leases instead of only the min lease. In theory, we only need to keep the image layers for each of the lease/branch LSN, as an future optimization.
pageserver/src/tenant/timeline.rs
Outdated
|
||
{ | ||
let mut gc_info = self.gc_info.write().unwrap(); | ||
gc_info.leases.insert(lsn, lease.clone()); |
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 feel there is something not optimal of the lease design. Imagine a compute node updates the lease every second and the default lease length is 300s, we will easily accumulate 300 leases in memory. I feel a better design is to have the lease request come with <compute_unique_id, lsn>
, so that one compute maintains exactly one lease in the system.
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.
something to be fixed in the future, could create an issue
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.
For this I was thinking that compute will basically consume the valid_until
expiration time and renew the lease a little bit before that?
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.
well, it depends on the cplane people, but I do feel usually heartbeats are designed in a way that the interval is 1/10 of the expiration interval.
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.
We keep the leases in a map of LSN->lease, right? A given compute will keep requesting the same LSN repeatedly, so the overall number of leases we track shouldn't increase.
pageserver/src/tenant/timeline.rs
Outdated
@@ -5010,6 +5036,10 @@ impl Timeline { | |||
// 1. it is older than cutoff LSN; | |||
// 2. it is older than PITR interval; | |||
// 3. it doesn't need to be retained for 'retain_lsns'; | |||
|
|||
// TODO(yuchen): we could consider current retain_lsns as infinite leases. |
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.
feel free to create a tracking issue if this is something you plan to work on in short term
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Part of #7497, extracts from #7996, closes #8063. ## Problem With the LSN lease API introduced in #7808, we want to implement the real lease logic so that GC will keep all the layers needed to reconstruct all pages at all the leased LSNs with valid leases at a given time. Signed-off-by: Yuchen Liang <yuchen@neon.tech>
The code changes and comments in this PR have been addressed in #8084. Closing it for now. |
Part of
Problem
This pull request is a PoC opening for early reviews and comments. The feedbacks will incorporate into later PRs.
With the LSN lease API introduced in #7808, we want to implement the real lease logic to prevent GC from proceeding pass some LSN when garbage collecting layers in a timeline.
Design doc at https://www.notion.so/neondatabase/LSN-Lease-Design-f8aa8333a9b7431d9905785ba7599745?pvs=4.
Summary of changes
GCInfo
.GCInfo
.retain_lsns
).Checklist before requesting a review
Checklist before merging