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

pageserver: fix vectored read aux key handling #7404

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

VladLazar
Copy link
Contributor

@VladLazar VladLazar commented Apr 17, 2024

Problem

Vectored get would descend into ancestor timelines for aux files.
This is not the behaviour of the legacy read path and blocks cutting
over to the vectored read path.

Fixes #7379

Summary of Changes

Treat non inherited keys specially in vectored get. At the point when
we want to descend into the ancestor mark all pending non inherited keys
as errored out at the key level. Note that this diverges from the
standard vectored get behaviour for missing keys which is a top level
error. This divergence is required to avoid blocking compaction in case
such an error is encountered when compaction aux files keys. I'm pretty
sure the bug I just described predates the vectored get implementation,
but it's still worth fixing.

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

@VladLazar VladLazar requested a review from a team as a code owner April 17, 2024 09:44
@VladLazar VladLazar requested review from arpad-m and skyzh and removed request for arpad-m April 17, 2024 09:44
@VladLazar VladLazar force-pushed the vlad/fix-vectored-read-aux-key-handling branch from 415e542 to 67e82c0 Compare April 17, 2024 09:45
Copy link

github-actions bot commented Apr 17, 2024

2772 tests run: 2654 passed, 0 failed, 118 skipped (full report)


Code coverage* (full report)

  • functions: 28.1% (6467 of 23043 functions)
  • lines: 46.9% (45795 of 97715 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
8835086 at 2024-04-23T11:19:05.334Z :recycle:

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

my main concern of this pull request is that we also have a is_inherited_key function, which serves almost the same purpose of NON_INHERITED_RANGE. I'm wondering if there could be some hints like "if you change is_inherited_key, also change NON_INHERITED_RANGE", or have a single place to define both things, so as to avoid problems in the future.

pageserver/src/tenant/timeline.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/timeline.rs Show resolved Hide resolved
pageserver/src/tenant/timeline.rs Outdated Show resolved Hide resolved
@VladLazar
Copy link
Contributor Author

my main concern of this pull request is that we also have a is_inherited_key function, which serves almost the same purpose of NON_INHERITED_RANGE. I'm wondering if there could be some hints like "if you change is_inherited_key, also change NON_INHERITED_RANGE", or have a single place to define both things, so as to avoid problems in the future.

Yeah that's true. Let's define is_inherited_key in terms of NON_INHERITED_RANGE: !NON_INHERITED_RANGE.contains(&key).

@VladLazar VladLazar force-pushed the vlad/fix-vectored-read-aux-key-handling branch from 67e82c0 to d961d84 Compare April 22, 2024 17:27
@VladLazar VladLazar requested a review from skyzh April 22, 2024 17:32
Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Rest LGTM

libs/pageserver_api/src/key.rs Show resolved Hide resolved
@skyzh skyzh mentioned this pull request Apr 22, 2024
5 tasks
Problem
Vectored get would descend into ancestor timelines for aux files.
This is not the behaviour of the legacy read path and blocks cutting
over to the vectored read path.

Summary of Changes
Treat non inherited keys specially in vectored get. At the point when
we want to descend into the ancestor mark all pending non inherited keys
as errored out at the key level. Note that this diverges from the
standard vectored get behaviour for missing keys which is a top level
error. This divergence is required to avoid blocking compaction in case
such an error is encountered when compaction aux files keys. I'm pretty
sure the bug I just described predates the vectored get implementation,
but it's still worth fixing.
@VladLazar VladLazar force-pushed the vlad/fix-vectored-read-aux-key-handling branch from d961d84 to 8835086 Compare April 23, 2024 10:32
Copy link
Member

@arpad-m arpad-m left a comment

Choose a reason for hiding this comment

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

We can merge this in my opinion, but I don't think it being a range helps us a lot: we'll need support for a list of ranges in the future instead of just one. That being said, refactoring from this state should not be harder than refactoring from a simpler version of this PR that bakes in the assumption that NON_INHERITED_RANGE is just one element large.

pageserver/src/tenant/timeline.rs Show resolved Hide resolved
@VladLazar VladLazar merged commit a9fda8c into main Apr 23, 2024
53 checks passed
@VladLazar VladLazar deleted the vlad/fix-vectored-read-aux-key-handling branch April 23, 2024 13:03
@VladLazar VladLazar mentioned this pull request Apr 23, 2024
5 tasks
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.

vectored get does not consider is_inherited_key
3 participants