schedule leaf searches by (priority, remaining_splits) lexicographically - #6695
schedule leaf searches by (priority, remaining_splits) lexicographically#6695Abdul-Andha wants to merge 4 commits into
Conversation
|
|
||
| // Scheduling priority for leaf search execution. Negative values are allowed, | ||
| // and lower values have higher priority. Callers that omit it get priority 0. | ||
| optional int32 priority = 21; |
There was a problem hiding this comment.
| optional int32 priority = 21; | |
| int32 priority = 21; |
No need for optional if we want to default to 0. Unless we want to semantically differentiate None from Some(0)
| // the raw IntermediateAggregationResults bytes instead. | ||
| bool skip_aggregation_finalization = 19; | ||
|
|
||
| reserved 20; |
There was a problem hiding this comment.
20 is used in pomsky. it will be easier to sync if we don't use 20, i think
![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/8305498a4764978cc4576db53ae0cefc4c5624bd/quickwit-proto/src/codegen/quickwit/quickwit.search.rs#L213-L215
Because SearchRequest derives Hash/Eq and LeafSearchCache stores the entire request in its CacheKey (leaf_cache.rs:84-114), this new field makes otherwise identical searches with different priorities occupy separate cache entries. Priority affects only scheduling, not search results, so workloads issuing the same query at different priorities lose valid cache hits, increase cache churn, and can make high-priority requests perform unnecessary split searches; normalize priority when constructing the cache key.
ℹ️ 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".
💡 Codex Reviewquickwit/quickwit/quickwit-search/src/search_permit_provider.rs Lines 234 to 240 in cf7ac28 Skip unserviceable priority heads
When the highest-priority request's next split exceeds the currently available memory, this ordering keeps that request at the heap head, and ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| task_metadata: Vec<SplitSearchTaskMetadata>, | ||
| ) -> (Self, Vec<SearchPermitFuture>) { | ||
| assert!(!task_metadata.is_empty(), "task_metadata must not be empty"); | ||
| let priority = task_metadata[0].priority; |
There was a problem hiding this comment.
it's a bit strange that all tasks must have the same priority, but there's still a per-task priority field
There was a problem hiding this comment.
i agree its a bit strange. i think the alternative is introducing a new struct for per-leafsearch metadata. what do you think?
struct LeafSearchTaskMetadata {
priority: i32,
splits: Vec<SplitSearchTaskMetadata>,
}
3 participants
Description
prioritytoSearchRequestLeafPermitRequestwas ordered by remaining_splits. Now it is ordered by (priority, remaining_splits)How was this PR tested?
test_search_permit_priority_precedes_remaining_splits