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

Add caching ObjectStore implementation #590

Closed
wants to merge 5 commits into from

Conversation

passaro
Copy link
Contributor

@passaro passaro commented Oct 31, 2023

Description of change

EDIT: This is now the follow-up to #592 which introduces a caching store implementation.

Still a draft PR to preview the use of the cache:

  • uses the InMemoryCache, which will be eventually replaced by a disk implementation,
  • CLI args are only temporary for initial testing.

Relevant issues: #255

Does this change impact existing behavior?

No. All changes hidden under caching flag.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@passaro passaro temporarily deployed to PR integration tests October 31, 2023 15:17 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 31, 2023 15:17 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 31, 2023 15:17 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 31, 2023 15:17 — with GitHub Actions Inactive
Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

Just a quick high-level pass, will try to get more detailed tomorrow. In particular, I haven't read cached_feed.rs yet. The general approach seems right to me though.

mountpoint-s3/src/fs.rs Show resolved Hide resolved
mountpoint-s3/src/main.rs Show resolved Hide resolved
mountpoint-s3/src/prefetch.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/prefetch/feed.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/store.rs Show resolved Hide resolved
mountpoint-s3/src/main.rs Show resolved Hide resolved
mountpoint-s3/src/main.rs Show resolved Hide resolved
mountpoint-s3/src/prefetch.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/prefetch/feed.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/store.rs Show resolved Hide resolved
Introduce a new `ObjectStore` trait that replaces `ObjectClient` in the mountpoint-s3 crate. In addition to most of `ObjectClient` methods, `ObjectStore` also declares a new `prefetch` method returning a `PrefetchGetObject` which allows callers to read the object content. `PrefetchGetObject` is where `ObjectStore` implementations can add object data caching.

This change also reworks the `Prefetcher` so that `ObjectStore` implementations can delegate `prefetch` to it. The main changes to `Prefetcher` are:
* it is now generic on the `ObjectPartStream` (previously `ObjectPartFeed`), rather than using dynamic dispatch.
* the logic to spawn a new task for each `GetObject` request and handle the object body parts returned was moved into `ObjectPartStream`.

Signed-off-by: Alessandro Passaro <[email protected]>
@passaro passaro force-pushed the object-store-cache branch from 70a1106 to e098a01 Compare November 1, 2023 16:39
@passaro passaro had a problem deploying to PR integration tests November 1, 2023 16:39 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 1, 2023 16:39 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 1, 2023 16:39 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 1, 2023 16:39 — with GitHub Actions Failure
@passaro passaro changed the title Introduce ObjectStore trait and add caching implementation Add caching ObjectStore implementation Nov 1, 2023
Signed-off-by: Alessandro Passaro <[email protected]>
Signed-off-by: Alessandro Passaro <[email protected]>
@passaro passaro force-pushed the object-store-cache branch from e098a01 to 1af9bf3 Compare November 2, 2023 11:52
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 11:52 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 11:52 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 11:52 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 11:52 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 15:50 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 15:50 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 15:50 — with GitHub Actions Failure
@passaro passaro had a problem deploying to PR integration tests November 2, 2023 15:50 — with GitHub Actions Failure
@passaro
Copy link
Contributor Author

passaro commented Nov 6, 2023

Replaced by #598

@passaro passaro closed this Nov 6, 2023
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.

3 participants