Multilevel cache #1064
Merged
Multilevel cache #1064
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
commented
Oct 15, 2024
muddyfish
reviewed
Oct 16, 2024
passaro
reviewed
Oct 16, 2024
vladem
force-pushed
the
multilevel-cache
branch
from
8d9efd6
to
6a8e202
Compare
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
muddyfish
reviewed
Nov 1, 2024
passaro
reviewed
Nov 1, 2024
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
vladem
force-pushed
the
multilevel-cache
branch
from
69d9063
to
a232279
Compare
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
reviewed
Nov 6, 2024
| MultilevelDataCache<DiskCache, ExpressCache, Runtime> | ||
| { | ||
| pub fn new(disk_cache: Arc<DiskCache>, express_cache: ExpressCache, runtime: Runtime) -> Self { | ||
| // Method `MultilevelDataCache::block_size` relies on block sizes of both caches to be equal. |
There was a problem hiding this comment.
A bit convoluted. We use the same blocks at both levels, so they need to have the same size. I'd mention it as a requirement in this method rustdoc.
mountpoint-s3/src/cli.rs
Outdated
| @@ -298,10 +309,10 @@ pub struct CliArgs { | |||
| #[cfg(feature = "block_size")] | |||
| #[clap( | |||
| long, | |||
| help = "Size of a cache block in KiB [Default: 1024 (1 MiB) for disk cache, 512 (512 KiB) for S3 Express cache]", | |||
| help = "Size of a cache block in KiB [Default: 1024 (1 MiB) for disk cache and for S3 Express cache]", | |||
There was a problem hiding this comment.
Suggested change
| help = "Size of a cache block in KiB [Default: 1024 (1 MiB) for disk cache and for S3 Express cache]", | |
| help = "Size of a cache block in KiB [Default: 1024 (1 MiB)]", |
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
approved these changes
Nov 6, 2024
| @@ -89,6 +91,7 @@ where | |||
| } | |||
| buffer.extend_from_slice(&body); | |||
|
|
|||
| // Ensure the flow-control window is large enough. | |||
| result.as_mut().increment_read_window(self.block_size as usize); | |||
There was a problem hiding this comment.
This seems unnecessary now, doesn't it?
There was a problem hiding this comment.
Happy to keep it for now and review when we optimize for the single chunk case (see TODO above).
There was a problem hiding this comment.
We'll need to account for a case when the block object is larger than block_size for some reason. If we've just removed this line the read may freeze. If we've kept it as it is now MP may attempt to read an unbounded amount of data to RAM.
This requires a bit more thinking, so I agree that it's better to address in the following PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Allow using both caches when
--cache-express <bucket> --cache <directory>options are specified, local cache is queried first.Relevant issues: No
Does this change impact existing behavior?
No.
Does this change need a changelog entry in any of the crates?
Yes, will add in one of the future PRs.
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).