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

feat: integrate lsn lease into synthetic size #8208

Closed
wants to merge 6 commits into from
Closed

feat: integrate lsn lease into synthetic size #8208

wants to merge 6 commits into from

Conversation

yliang412
Copy link
Contributor

@yliang412 yliang412 commented Jun 28, 2024 •
edited
Loading

Part of #7497, closes #8071.

Problem

After the changes in #8084, we need synthetic size to also account for leased LSNs so that users do not get free retention by running a small ephemeral endpoint for a long time.

Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We model leases as read-only branches started at the leased LSN (except it does not have a timeline id).

Other changes:

  • Add new unit tests testing whether a lease behaves like a read-only branch.
  • Change /size_debug response to include lease point in the SVG visualization.
  • Fix /lsn_lease HTTP API to do proper parsing for POST.

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

yliang412 and others added 4 commits June 28, 2024 18:13
@yliang412
feat: integrate lsn lease into synthetic size
10c46db 
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
@yliang412
Merge branch 'main' into yuchen/remove-timeline-deps-for-synthetic-size
542e7ed
@yliang412
remove scratch
3c8611f 
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
@yliang412
remove scratch testing
911da0a 
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
@yliang412
Copy link
Contributor Author

Leases vs. Equivalent RO Timeline

Leases
image

RO Timeline
image

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 28, 2024 •
edited
Loading

3012 tests run: 2897 passed, 0 failed, 115 skipped (full report)

Code coverage* (full report)

  • functions: 32.7% (6913 of 21165 functions)
  • lines: 50.0% (54210 of 108383 lines)

* collected from Rust tests only

The comment gets automatically updated with the latest test results
5eb37a2 at 2024-07-01T14:18:15.588Z :recycle:

@yliang412
Copy link
Contributor Author

yliang412 commented Jul 1, 2024 •
edited
Loading

Regarding test_lsn_lease_size[False] failure on PG16 (allure):

The threshold set in assert_size_approx_equal is based on empirical equality checks, maybe it's larger this time?

yliang412
yliang412 commented Jul 1, 2024
View reviewed changes
libs/tenant_size_model/src/svg.rs
@@ -6,13 +6,13 @@ const SVG_WIDTH: f32 = 500.0;
struct SvgDraw<'a> {
storage: &'a StorageModel,
branches: &'a [String],
seg_to_branch: &'a [usize],
seg_to_branch: &'a [(usize, bool)],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't find a good way to propagate the info regarding whether a point is a lease.

@yliang412
remove dbg print
796bd7c 
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
@yliang412 yliang412 force-pushed the yuchen/remove-timeline-deps-for-synthetic-size branch from 4323ba6 to 796bd7c Compare July 1, 2024 13:16
@yliang412 yliang412 marked this pull request as ready for review July 1, 2024 13:17
@yliang412 yliang412 requested a review from a team as a code owner July 1, 2024 13:17
@yliang412 yliang412 requested review from jcsp, problame and koivunej and removed request for jcsp July 1, 2024 13:17
@yliang412 yliang412 changed the title (WIP) feat: integrate lsn lease into synthetic size feat: integrate lsn lease into synthetic size Jul 1, 2024
@yliang412 yliang412 self-assigned this Jul 1, 2024
@yliang412 yliang412 closed this Jul 1, 2024
@yliang412 yliang412 deleted the yuchen/remove-timeline-deps-for-synthetic-size branch July 1, 2024 14:38
@yliang412 yliang412 mentioned this pull request Jul 1, 2024
Merged
5 tasks
yliang412 added a commit that referenced this pull request Jul 4, 2024
@yliang412 @koivunej @problame
feat(pageserver): integrate lsn lease into synthetic size (#8220)
19accfe 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
@yliang412 @koivunej
@problame @VladLazar
feat(pageserver): integrate lsn lease into synthetic size (#8220)
8e67092 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
@yliang412 @koivunej
@problame @VladLazar
feat(pageserver): integrate lsn lease into synthetic size (#8220)
f6abef5 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
@yliang412 @koivunej
@problame @VladLazar
feat(pageserver): integrate lsn lease into synthetic size (#8220)
e5cf5be 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
@yliang412 @koivunej
@problame @VladLazar
feat(pageserver): integrate lsn lease into synthetic size (#8220)
194f4b2 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
@yliang412 @koivunej
@problame @VladLazar
feat(pageserver): integrate lsn lease into synthetic size (#8220)
82892ad 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
@yliang412 @koivunej
@problame @VladLazar
feat(pageserver): integrate lsn lease into synthetic size (#8220)
32828cd 
Part of #7497, closes #8071. (accidentally closed #8208, reopened here)

## Problem

After the changes in #8084, we need synthetic size to also account for
leased LSNs so that users do not get free retention by running a small
ephemeral endpoint for a long time.

## Summary of changes

This PR integrates LSN leases into the synthetic size calculation. We
model leases as read-only branches started at the leased LSN (except it
does not have a timeline id).

Other changes:
- Add new unit tests testing whether a lease behaves like a read-only
branch.
- Change `/size_debug` response to include lease point in the SVG
visualization.
- Fix `/lsn_lease` HTTP API to do proper parsing for POST.



Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Co-authored-by: Christian Schwarz <christian@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@problame problame Awaiting requested review from problame

@koivunej koivunej Awaiting requested review from koivunej

Assignees

@yliang412 yliang412

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update synthetic size calculation to account for leases.
1 participant
@yliang412