Store extra metadata in splits for disaster recovery - #6689
Conversation
a63634d to
17af55c
Compare
17af55c to
4248627
Compare
4248627 to
52ed858
Compare
![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
https://github.com/quickwit-oss/quickwit/blob/52ed8587277078e2b89ca0056b98a843d9f60f79/quickwit-metastore/src/split_metadata.rs#L232
When a disaster-recovery importer publishes the SplitMetadata returned here, every restored split has an empty node ID. merge_planner::belongs_to_pipeline requires an exact node-ID match, so these splits never belong to any real merge pipeline and immature recovered splits will no longer be compacted. Preserve the original node ID in the recovery payload or require the importer to supply a target node ID during reconstruction.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
52ed858 to
d16610a
Compare
| counters.num_staged_splits.fetch_add(split_metadata_list.len() as u64, Ordering::SeqCst); | ||
| counters | ||
| .num_staged_splits | ||
| .fetch_add(split_metadata_list.len() as u64, Ordering::SeqCst); |
There was a problem hiding this comment.
nit (not related to this PR): we don't need Ordering::SeqCst for a counter, a Ordering::Relaxed is enough.
| /// ```text | ||
| /// [bundle metadata (`BundleFileRanges`)][bundle metadata length] | ||
| /// ``` | ||
| pub async fn read_split_footer( |
There was a problem hiding this comment.
For trailer-based splits locate_split_footer_range validates the absolute footer start but not the footer's internal structure. A malformed trailer or corrupted nested length can therefore cause an invalid slice access in read_split_footer and to panic.
The above is fine during normal operations or disaster recovery from an intact object store.
However, it could make disaster recovery harder to achieve in case some of splits gets corrupted.
I don't think any changes should be made to this PR but it's worth pointing it out.
|
|
||
| fn deserialize_split_footer_trailer(trailer: &[u8]) -> anyhow::Result<Option<u64>> { | ||
| if trailer.len() != SPLIT_FOOTER_TRAILER_NUM_BYTES | ||
| || &trailer[12..] != SPLIT_FOOTER_TRAILER_MAGIC |
There was a problem hiding this comment.
nit: same comment as above on manual slices indexes and the potential usage of Reader/bytes::Buf.
Mallets
left a comment
Mallets
left a comment
There was a problem hiding this comment.
LGTM
I validated this PR on a live cluster and all the necessary metadata are stored in the splits, resulting in a correct metastore backfilling.
2 participants
Description
Store extra metadata in splits so one can (somewhat) recover the metastore database from storage.
How was this PR tested?
Add unit tests and integrations
Tested locally: