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: remove legacy read path #8601

Merged
merged 9 commits into from
Aug 6, 2024
Merged

Conversation

VladLazar
Copy link
Contributor

Problem

We have been maintaining two read paths (legacy and vectored) for a while now. The legacy read-path
was only used for cross validation in some tests.

Summary of changes

  • Tweak all tests that were using the legacy read path to use the vectored read path instead
  • Remove the read path dispatching based on the pageserver configs
  • Remove the legacy read path code

We will be able to remove the single blob io code in pageserver/src/tenant/blob_io.rs when
#7386 is complete.

Closes #8005

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

Copy link

github-actions bot commented Aug 5, 2024

2108 tests run: 2039 passed, 0 failed, 69 skipped (full report)


Flaky tests (1)

Postgres 16

  • test_scrubber_physical_gc_ancestors_split: debug

Code coverage* (full report)

  • functions: 32.8% (7124 of 21690 functions)
  • lines: 50.6% (57491 of 113730 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
87e3c1e at 2024-08-05T11:59:19.024Z :recycle:

@VladLazar VladLazar force-pushed the vlad/remove-legacy-read-path branch from 2a1564b to 2891818 Compare August 5, 2024 10:50
@VladLazar VladLazar marked this pull request as ready for review August 5, 2024 11:50
@VladLazar VladLazar requested a review from a team as a code owner August 5, 2024 11:50
@VladLazar VladLazar requested review from arpad-m and skyzh August 5, 2024 11:50
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.

LGTM. It seems that we didn't remove read path configurations like DEFAULT_GET_IMPL in this pull request? I assume there will be a follow up PR to completely remove them later.

@arpad-m
Copy link
Member

arpad-m commented Aug 5, 2024

We will be able to remove the single blob io code in pageserver/src/tenant/blob_io.rs when #7386 is complete.

As a note, I think it would be useful to move and keep the the tests in blob_io.rs.

Also, it might be useful to eventually remove the _vectored postfix from names as it is the new default.

@VladLazar
Copy link
Contributor Author

It seems that we didn't remove read path configurations like DEFAULT_GET_IMPL in this pull request? I assume there will be a follow up PR to completely remove them later.

Yes, I will do the configs in a follow-up

@VladLazar VladLazar merged commit 44fedfd into main Aug 6, 2024
64 checks passed
@VladLazar VladLazar deleted the vlad/remove-legacy-read-path branch August 6, 2024 09:14
jcsp pushed a commit that referenced this pull request Aug 12, 2024
## Problem

We have been maintaining two read paths (legacy and vectored) for a
while now. The legacy read-path was only used for cross validation in some tests.

## Summary of changes
* Tweak all tests that were using the legacy read path to use the
vectored read path instead
* Remove the read path dispatching based on the pageserver configs
* Remove the legacy read path code

We will be able to remove the single blob io code in
`pageserver/src/tenant/blob_io.rs` when #7386 is complete.

Closes #8005
VladLazar added a commit that referenced this pull request Aug 12, 2024
It has been virtuall unused since #8601
VladLazar added a commit that referenced this pull request Aug 13, 2024
## Problem
Pageserver exposes some vectored get related configs which are not in
use.

## Summary of changes
Remove the following pageserver configs: get_impl, get_vectored_impl,
and `validate_get_vectored`.
They are not used in the pageserver since
#8601.
Manual overrides have been removed from the aws repo in
neondatabase/infra#1664.
problame added a commit that referenced this pull request Dec 13, 2024
…global`

As of commit "pageserver: remove legacy read path" (#8601) we always use
vectored get, which has a separate metric.
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.

Remove validation code and non-vectored read path
3 participants