Optimize split recovery metadata reads from split bundles - #6723
Conversation
Reuse tail bytes when possible to avoid opening the full bundle and reduce object storage requests.
Mallets
changed the title
Mallets
changed the title
|
/ci-run-all-tests |
|
|
||
| // Start with the requested window, but always cover the fixed trailer and never read before | ||
| // the beginning of the split. | ||
| let initial_tail_num_bytes = initial_tail_window_num_bytes |
There was a problem hiding this comment.
nitpick. I find it always max/min have some conterintuitive aspect.
We think: the MAXIMUM accepted value is split_len, so we counterintuitively need to compute
x.min(split_len).
initial_tail_window_num_bytes.clamp() helps with that.
Granted... this goes against my common rant about the proliferation of needless methods in rust's std.
There was a problem hiding this comment.
I wasn't really aware of clamp(). Indeed it helps here. Done in 575229a.
| Ok(footer_start..split_len) | ||
| } | ||
|
|
||
| fn locate_split_footer_range_in_tail( |
There was a problem hiding this comment.
why is this so complicated? Is it explained somewhere?
Has there been a change of format?
There was a problem hiding this comment.
Yes, the format has been changed in #6689 and it is explained in:
Note that this PR does not modify the format.
| storage: &dyn Storage, | ||
| split_path: &Path, | ||
| split_len: u64, | ||
| initial_tail_window_num_bytes: u64, |
There was a problem hiding this comment.
Is initial_tail_window_num_bytes useful as an argument? I only see a const being used?
There was a problem hiding this comment.
In the context of fetch_split_tail standalone function, I believe it's good to accept it as a parameter and it's the caller's responsibility to decide how big the initial_tail_window should be. The caller now uses a const, but any change in the logic of the caller shouldn't result in a change in the fetch_split_tail function itself.
There was a problem hiding this comment.
This is not a valid point, is it?
The function is private. The value will stay const forever and I don't see any reason to make it the caller responsibility to pick the value, apart from increasing complexity for no reason at all:
that function contract is needlessly complicated as a result.
| /// The split length is provided by the caller (e.g. from object listing metadata) to avoid a | ||
| /// separate metadata request. | ||
| pub async fn fetch_file_from_split( | ||
| storage: Arc<dyn Storage>, |
| /// separate metadata request. | ||
| pub async fn fetch_file_from_split( | ||
| storage: Arc<dyn Storage>, | ||
| bundle_filepath: PathBuf, |
There was a problem hiding this comment.
| bundle_filepath: PathBuf, | |
| bundle_filepath: &Path, |
| /// separate metadata request. | ||
| pub async fn fetch_file_from_split( | ||
| storage: Arc<dyn Storage>, | ||
| bundle_filepath: PathBuf, |
There was a problem hiding this comment.
bundle filepath? So the manifest is just another file of the bundle I presume.
But the implementation is very specifically targeting that we append at the very end right?
Shouldn't we specialize it to only apply to manifest files? (i don't know how you called them)
There was a problem hiding this comment.
Ah I guess this is not necessary. Worst case scenario we get it into fetch. Ok fair enough.
| /// Use this only when retrieving a single file from the split. To retrieve multiple files, | ||
| /// prefer [`Self::open_from_storage`]. | ||
| /// | ||
| /// The split length is provided by the caller (e.g. from object listing metadata) to avoid a |
There was a problem hiding this comment.
can you document the result semantics because this is not clear at all.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a12f3941c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4 participants
Summary
Disaster recovery only needs the recovery metadata stored in each split, but it may need to inspect a large number of splits. Separate requests to discover each split’s size, locate its footer, and fetch the recovery metadata would multiply object-storage GETs across the bucket.
Bucket listings already provide each split’s object length, so callers can pass it directly and avoid a separate metadata request. We then optimistically read the final
1 MiBof the split. In the expected case, this contains the complete footer and recovery metadata, allowing retrieval with a single GET per split.Details
BundleStorage::fetch_file_from_splitfor retrieving one bundled file when the split length is known.Test plan
make fmtcargo clippy -p quickwit-storage --all-features --testscargo nextest run -p quickwit-storage --all-features bundle_storage