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 |
| // If the initial tail also contains the file, reuse it and complete in one GET (at least). | ||
| // Otherwise, fetch the file with an additional GET. | ||
| let file_bytes = if file_range.start >= tail_start { | ||
| let relative_start = usize::try_from(file_range.start - tail_start)?; |
There was a problem hiding this comment.
Technically it will never fail there, it will fail first in the fetch_split_tail.
The failure case is when running on 32 bits and the split is larger than 4GB. However, I'm not sure either that the whole system can run on 32 bits, in that case these faillable conversions can be replaced with a simple cast.
There was a problem hiding this comment.
AI generates crazy defensive code like this.
Last week Cong had a PR with u64::try_from(usize),
and codex keep generating thing like this for me too.
I don't think we need to accept those. It hurts readability IMHO
There was a problem hiding this comment.
Yeah, u64::try_from(usize) is non-sense. But for faillable conversions on supported platforms it makes sense to me. Said that, we can consider 32 bits as a non-supported target.
Done in 5947c79.
| ensure!( | ||
| initial_tail_window_num_bytes > 0, | ||
| "split tail window must be positive" | ||
| ); |
| .max(SPLIT_FOOTER_TRAILER_NUM_BYTES as u64) | ||
| .min(split_len); | ||
| // Read the tail in a loop until we find the footer. | ||
| let (tail_bytes, footer_range) = loop { |
There was a problem hiding this comment.
if possible avoid
- loop without clear exit condition. It makes proofreading hard and is error prone.
- break with value
There was a problem hiding this comment.
here... I don't understand why this is a loop?
There was a problem hiding this comment.
Done in 904fcfb.
loop is a way to retry the exact operation twice. Fine to make it explicit here.
|
|
||
| fn locate_split_footer_range_in_tail( | ||
| split_len: u64, | ||
| tail_bytes: &OwnedBytes, |
There was a problem hiding this comment.
| tail_bytes: &OwnedBytes, | |
| tail_bytes: &[u8], |
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