Draft Fix chunk cache key and metadata retrieval #104
Conversation
This file contains hidden or 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
…nloaded byte range under a key that doesn't account for this range. When another request is received we get a cache hit even though the requested byte range may differ. Fix this by downloading and caching the entire chunk, applying the byte range post download or cache hit. If caching is disabled honour the byte range in the S3 client download.
…ache chunks aren't shared between requests of differing byte range. Revert the previous fix which suffers from memory over consumption when bombarded by requests for byte ranges from the same very large file - we would need a way to ensure a single download of that file before servicing concurrent requests against it.
1) Only incorporate request fields that themselves are used in the S3 object download 2) Immediately turn this key into a md5 hash so we're handling a much shorter string when interacting with the cache
…essing task to determine if a chunk is cached and if so its size. There's a MPSC channel used to buffer all cache write requests to a single task responsible for cache updates but the update task could happen at the same time request processing tasks read the state. Reading the state file whilst it's being written results in a serde parsing error. Instead of adding some type of thread safety around the load/save of the state file a simpler solution is to store a chunk's metadata in its own metadata file. This mirrors the chunk get which ignores the state file and simply retrieves files from disk.
sd109
requested changes
Apr 17, 2025
There was a problem hiding this comment.
Nice, seems like a sensible approach to me.
Do we also need to be worried about the pruning cycle trying to access the cache state file while the cache is being written to? I see we're using load_state in the remove method as well as in various places throughout the prune* methods.
src/chunk_cache.rs
Outdated
| @@ -320,6 +319,26 @@ impl SimpleDiskCache { | |||
| } | |||
| } | |||
|
|
|||
| /// Retrieves chunk metadata from the cache for an unique key. | |||
There was a problem hiding this comment.
Nit:
Suggested change
| /// Retrieves chunk metadata from the cache for an unique key. | |
| /// Retrieves chunk metadata from the cache for a unique key. |
src/chunk_cache.rs
Outdated
Comment on lines
389
to
395
| fs::remove_file(path.join(self.filename_for_key(key).await)) | ||
| .await | ||
| .unwrap(); | ||
| // Remove the metadata file | ||
| fs::remove_file(path.join(self.filename_for_key(key).await + ".meta")) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Is there anything we can do to avoid the unwraps here? Or we could at least log the error for easier debugging with something like
Suggested change
| fs::remove_file(path.join(self.filename_for_key(key).await)) | |
| .await | |
| .unwrap(); | |
| // Remove the metadata file | |
| fs::remove_file(path.join(self.filename_for_key(key).await + ".meta")) | |
| .await | |
| .unwrap(); | |
| fs::remove_file(path.join(self.filename_for_key(key).await)) | |
| .await | |
| .map_err(|e| log::error!("Failed to remove cache data with: {}", e) | |
| .unwrap(); | |
| // Remove the metadata file | |
| fs::remove_file(path.join(self.filename_for_key(key).await + ".meta")) | |
| .await | |
| .map_err(|e| log::error!("Failed to remove cache metadata with: {}", e) | |
| .unwrap(); |
N.B. We might also want to add this map_error pattern anywhere else in the code that we need to use unwrap / expect.
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.
Fixes for the chunk cache: