Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Grant optional lease to get_lsn_by_timestamp request #8072

Closed
Tracked by #7497
yliang412 opened this issue Jun 17, 2024 · 4 comments · Fixed by #8104
Closed
Tracked by #7497

Grant optional lease to get_lsn_by_timestamp request #8072

yliang412 opened this issue Jun 17, 2024 · 4 comments · Fixed by #8104
Assignees
@yliang412
Labels
c/storage/pageserver Component: storage: pageserver t/feature Issue type: feature, for new features or requests

Comments

@yliang412
Copy link
Contributor

yliang412 commented Jun 17, 2024 •
edited
Loading

Part of #7497.

An with_lease field is added to the get_lsn_by_timestamp request so we can optionally grant leases to valid LSNs eligible for branch creation.
@yliang412 yliang412 mentioned this issue Jun 17, 2024
Closed
@yliang412 yliang412 self-assigned this Jun 17, 2024
@yliang412 yliang412 added t/feature Issue type: feature, for new features or requests c/storage/pageserver Component: storage: pageserver labels Jun 17, 2024
@skyzh
Copy link
Member

skyzh commented Jun 17, 2024

should there be a new field grant_lease for this API call, or making all get_lsn_by_timestamp a lease request?

@yliang412
Copy link
Contributor Author

should there be a new field grant_lease for this API call, or making all get_lsn_by_timestamp a lease request?

Based on the slack discussion, I'm inclined to add grant_lease as an optional request parameter (default being no lease). If later we think granting lease should always be the case, we can then change the default to always grant the lease?

@skyzh
Copy link
Member

skyzh commented Jun 17, 2024

sounds good

@yliang412 yliang412 changed the title Grant implicit lease to get_lsn_by_timestamp request Grant optional lease to get_lsn_by_timestamp request Jun 18, 2024
@yliang412 yliang412 mentioned this issue Jun 18, 2024
Merged
5 tasks
@yliang412
Copy link
Contributor Author

Last week: finished implementation.

This week: merge PR after metrics are added in.

conradludgate pushed a commit that referenced this issue Jun 27, 2024
@yliang412 @conradludgate
feat(pageserver): add an optional lease to the get_lsn_by_timestamp A…
76fc580 
…PI (#8104)

Part of #7497, closes #8072.

## Problem

Currently the `get_lsn_by_timestamp` and branch creation pageserver APIs do not provide a pleasant client experience where the looked-up LSN might be GC-ed between the two API calls.

This PR attempts to prevent common races between GC and branch creation by making use of LSN leases provided in #8084. A lease can be optionally granted to a looked-up LSN. With the lease, GC will not touch layers needed to reconstruct all pages at this LSN for the duration of the lease.

Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yliang412 yliang412

Labels
c/storage/pageserver Component: storage: pageserver t/feature Issue type: feature, for new features or requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(pageserver): add an optional lease to the get_lsn_by_timestamp API neondatabase/neon
2 participants
@skyzh @yliang412