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

Remove BlockReader::read_blk in favour of BlockCursor #5015

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

arpad-m
Copy link
Member

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

Problem

We want to make read_blk an async function, but outside of async_trait, which allocates, and nightly features, we can't use async fn's in traits.

Summary of changes

  • Remove all uses of BlockReader::read_blk in favour of using block cursors.
  • Introduce a BlockReaderRef enum that lists all implementors of BlockReader::read_blk.
  • Remove BlockReader::read_blk and move its implementations into inherent functions on the types instead.

We don't turn read_blk into an async fn yet, for that we also need to modify the page cache. So this is a preparatory PR, albeit an important one.

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 16, 2023 21:23
@arpad-m arpad-m requested review from fprasx, problame and koivunej and removed request for a team August 16, 2023 21:23
@github-actions
Copy link

github-actions bot commented Aug 16, 2023

1624 tests run: 1551 passed, 0 failed, 73 skipped (full report)


The comment gets automatically updated with the latest test results
a1f0143 at 2023-08-25T08:50:14.871Z :recycle:

@arpad-m
Copy link
Member Author

arpad-m commented Aug 18, 2023

Rebased after merge of #4994

@arpad-m arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch from 7c106f7 to 8d3b0d3 Compare August 18, 2023 19:18
@arpad-m arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch from 8d3b0d3 to d63f31d Compare August 23, 2023 15:47
At least in the cases where the concrete type won't be known clearly.
Also, require BlockReader implementors to impl block_cursor.
@arpad-m arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch from d63f31d to 3719b16 Compare August 24, 2023 21:00
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Adapter, DeltaLayerInner::load_keys now have extra code, but this should be ok to leave, I'll clean them up.

@arpad-m arpad-m merged commit 8c13296 into main Aug 25, 2023
@arpad-m arpad-m deleted the arpad/pageserver_cursor_read_blk branch August 25, 2023 10:28
problame pushed a commit that referenced this pull request Aug 30, 2023
## Problem

`read_blk` does I/O and thus we would like to make it async. We can't
make the function async as long as the `PageReadGuard` returned by
`read_blk` isn't `Send`. The page cache is called by `read_blk`, and
thus it can't be async without `read_blk` being async. Thus, we have a
circular dependency.

## Summary of changes

Due to the circular dependency, we convert both the page cache and
`read_blk` to async at the same time:

We make the page cache use `tokio::sync` synchronization primitives as
those are `Send`. This makes all the places that acquire a lock require
async though, which we then also do. This includes also asyncification
of the `read_blk` function.

Builds upon #4994, #5015, #5056, and #5129.

Part of #4743.
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