Make MergeSchedulerService optional#6266
Merged
nadav-govari merged 4 commits intonadav/feature-split-mergesfrom Apr 6, 2026
Merged
Make MergeSchedulerService optional#6266nadav-govari merged 4 commits intonadav/feature-split-mergesfrom
nadav-govari merged 4 commits intonadav/feature-split-mergesfrom
Conversation
guilload
approved these changes
Apr 6, 2026
| metastore: MetastoreServiceClient, | ||
| ingest_api_service_opt: Option<Mailbox<IngestApiService>>, | ||
| merge_scheduler_service: Mailbox<MergeSchedulerService>, | ||
| merge_scheduler_service: Option<Mailbox<MergeSchedulerService>>, |
Member
There was a problem hiding this comment.
Suggested change
| merge_scheduler_service: Option<Mailbox<MergeSchedulerService>>, | |
| merge_scheduler_service_opt: Option<Mailbox<MergeSchedulerService>>, |
| merge_io_throughput_limiter_opt: self.merge_io_throughput_limiter_opt.clone(), | ||
| max_concurrent_split_uploads: self.max_concurrent_split_uploads, | ||
| event_broker: self.event_broker.clone(), | ||
| let merge_planner_mailbox = if let Some(merge_scheduler) = |
Member
There was a problem hiding this comment.
Suggested change
| let merge_planner_mailbox = if let Some(merge_scheduler) = | |
| let merge_planner_mailbox = if let Some(merge_scheduler_service) = |
| // When merging is handled locally, notify the merge planner about new | ||
| // splits. The mailbox is None when an external merge service is active, | ||
| // or when the planner has already shut down (e.g. source reached its end). | ||
| if let Some(merge_planner_mailbox) = self.merge_planner_mailbox_opt.as_ref() { |
Member
There was a problem hiding this comment.
Suggested change
| if let Some(merge_planner_mailbox) = self.merge_planner_mailbox_opt.as_ref() { | |
| if let Some(merge_planner_mailbox) = &self.merge_planner_mailbox_opt { |
| // splits. The mailbox is None when an external merge service is active, | ||
| // or when the planner has already shut down (e.g. source reached its end). | ||
| if let Some(merge_planner_mailbox) = self.merge_planner_mailbox_opt.as_ref() { | ||
| let _ = ctx |
Member
There was a problem hiding this comment.
Not your code but I'd like the see an error! log statement error when we can't send the message to the merge planner.
Member
|
Let's make sure you spin this up locally and merges are running. |
Collaborator
Author
|
Verified on a local quickwit instance that merges start up and work correctly. |
guilload
reviewed
Apr 6, 2026
Comment on lines
+217
to
+225
| match ctx | ||
| .send_message(merge_planner_mailbox, NewSplits { new_splits }) | ||
| .await; | ||
| .await | ||
| { | ||
| Ok(_) => {} | ||
| Err(error) => { | ||
| error!(error=?error, "failed to send new splits to merge planner"); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| match ctx | |
| .send_message(merge_planner_mailbox, NewSplits { new_splits }) | |
| .await; | |
| .await | |
| { | |
| Ok(_) => {} | |
| Err(error) => { | |
| error!(error=?error, "failed to send new splits to merge planner"); | |
| } | |
| } | |
| if let Err(error) = ... { | |
| error!(error, "failed to send splits to merge planner"); | |
| } |
nit: for next time
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
This is step 1 of the plan to create a new merge service to split merging out of indexing pipelines.
In this plan, a new quickwit service called merge-planner will be introduced to handle all merges.
Similar to how the control plane is optionally started, an indexer node will try to connect to the merge planner on startup. If it does, it'll pass None to the IndexingService, which will not spawn merge pipelines anymore. If it can't connect, the IndexingService will be started up with a MergeSchedulerService, which is the signal to performs merges locally (as it's done today).
In this PR, the merge scheduler service is made optional on indexing pipelines and propagated through as needed.
How was this PR tested?
Unit tests. Nothing intrusive has been done yet.