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

Turn BlockLease associated type into an enum #4982

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Aug 13, 2023

Problem

The BlockReader trait is not ready to be asyncified, as associated types are not supported by asyncification strategies like via the async_trait macro, or via adopting enums.

Summary of changes

Remove the BlockLease associated type from the BlockReader trait and turn it into an enum instead, bearing the same name. The enum has two variants, one of which is gated by #[cfg(test)]. Therefore, outside of test settings, the enum has zero overhead over just having the PageReadGuard. Using the enum allows us to impl BlockReader without needing the page cache.

Part of #4743

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

@arpad-m arpad-m requested review from a team as code owners August 13, 2023 14:02
@arpad-m arpad-m requested review from fprasx and LizardWizzard and removed request for a team August 13, 2023 14:02
@github-actions
Copy link

github-actions bot commented Aug 13, 2023

1588 tests run: 1513 passed, 0 failed, 75 skipped (full report)


The comment gets automatically updated with the latest test results
b1f1c10 at 2023-08-14T11:49:51.881Z :recycle:

Copy link
Contributor

@LizardWizzard LizardWizzard 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

@arpad-m arpad-m merged commit baf3959 into main Aug 14, 2023
@arpad-m arpad-m deleted the arpad/pageserver_io_async_block_reader branch August 14, 2023 16:48
problame added a commit that referenced this pull request Aug 16, 2023
It was necessary before I rebased this patch on top of

commit baf3959
Author: Arpad Müller <arpad-m@users.noreply.github.com>
Date:   Mon Aug 14 18:48:09 2023 +0200

    Turn BlockLease associated type into an enum (#4982)
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.

2 participants