Rework merge task for both compactor and indexer merge flows#6464
Conversation
![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: a9fa5cabc0
ℹ️ 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".
| let merge_task_opt = match merge_source { | ||
| MergeSource::Task(task) => Some(task), | ||
| MergeSource::Operation(_) => None, |
There was a problem hiding this comment.
When this branch forwards a scheduled MergeTask, it eventually lands in SplitsUpdate::merge_task, but the logs publisher destructures SplitsUpdate with .. at quickwit/quickwit-indexing/src/actors/log_publisher_impl.rs:68-75, which drops that field before publish_splits is awaited. For the old merge/delete pipelines, this releases the scheduler permit and removes the operation from the planner/delete inventory before the replacement is actually committed, so a slow or failed publish can let another merge be scheduled and make known_split_ids GC treat the input splits as no longer in flight. Hold/drop the task only after publish succeeds, as the parquet publisher does, if this forwarding is meant to preserve merge-task lifetime.
Useful? React with 👍 / 👎.
nadav-govari
merged commit 7054e1d
into
nadav/merge-feature-split-merges
1 participant
Description
One thing that was done in the feature branch was reworking the MergeScratch used in the merge pipeline with an eye on deprecating MergeTask, since I thought that was going away. Since the existing flow stays for now, we need the merge task, which tracks merge permits per pipeline for the existing flow. So rework it into an enum thats either merge task or merge operation.
How was this PR tested?
Unit tests updated.