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.

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): add an optional lease to the get_lsn_by_timestamp API #8104

Merged
merged 9 commits into from
Jun 24, 2024

Conversation

yliang412
Copy link
Contributor

@yliang412 yliang412 commented Jun 18, 2024

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.

Caveats: Leases can only prevent the common case. Branch creation at a LSN after lease is expired can still race with GC.

Summary of changes

  • Added an optional with_lease field in the request and an optional valid_until field in the response to optionally grant leases.
  • Revised OpenAPI spec for get_lsn_by_timestamp.
  • Modified test_lsn_mapping to parametrize over with_lease.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
@yliang412 yliang412 marked this pull request as ready for review June 18, 2024 20:32
@yliang412 yliang412 requested a review from a team as a code owner June 18, 2024 20:32
@yliang412 yliang412 requested a review from VladLazar June 18, 2024 20:32
Copy link

github-actions bot commented Jun 18, 2024

2922 tests run: 2806 passed, 0 failed, 116 skipped (full report)


Code coverage* (full report)

  • functions: 32.6% (6883 of 21133 functions)
  • lines: 50.2% (53658 of 106976 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
022eecc at 2024-06-24T20:06:24.042Z :recycle:

Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I also peeked at the lease implementation and was surprised the leases are not persisted to disk anywhere. Are we ok with losing them on restart?

pageserver/src/http/routes.rs Show resolved Hide resolved
test_runner/fixtures/pageserver/http.py Outdated Show resolved Hide resolved
pageserver/src/http/openapi_spec.yml Show resolved Hide resolved
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
@yliang412
Copy link
Contributor Author

TODO: Investigate how get_lsn_by_timestamp API is used. How often is a lease not needed? This will help us decide whether we want the default behavior to grant a lease (or not).

@yliang412 yliang412 enabled auto-merge (squash) June 24, 2024 19:24
@yliang412 yliang412 merged commit 219e78f into main Jun 24, 2024
57 checks passed
@yliang412 yliang412 deleted the yuchen/get-lsn-by-ts-leases branch June 24, 2024 20:12
conradludgate pushed a commit that referenced this pull request Jun 27, 2024
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grant optional lease to get_lsn_by_timestamp request
2 participants