-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH] Eventual consistency in query nodes #5945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ use chroma_sysdb::{GetCollectionsOptions, SysDb}; | |
| use chroma_system::System; | ||
| use chroma_types::{ | ||
| operator::{Filter, KnnBatch, KnnProjection, Limit, Projection, Scan}, | ||
| plan::{Count, Get, Knn, Search}, | ||
| plan::{Count, Get, Knn, ReadLevel, Search}, | ||
| AddCollectionRecordsError, AddCollectionRecordsRequest, AddCollectionRecordsResponse, | ||
| AttachFunctionRequest, AttachFunctionResponse, Cmek, Collection, CollectionUuid, | ||
| CountCollectionsError, CountCollectionsRequest, CountCollectionsResponse, CountRequest, | ||
|
|
@@ -1834,6 +1834,7 @@ impl ServiceBasedFrontend { | |
| collection_and_segments, | ||
| }, | ||
| payloads: request.searches, | ||
| read_level: ReadLevel::IndexAndWal, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Requirements] If Context for Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is deliberate and handled up-stack |
||
| }; | ||
|
|
||
| // Execute the single search plan using the executor | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,8 @@ use chroma_system::{ | |
| OrchestratorContext, PanicError, TaskError, TaskMessage, TaskResult, | ||
| }; | ||
| use chroma_types::{ | ||
| operator::Filter, CollectionAndSegments, HnswParametersFromSegmentError, SchemaError, | ||
| SegmentType, | ||
| operator::Filter, plan::ReadLevel, CollectionAndSegments, HnswParametersFromSegmentError, | ||
| SchemaError, SegmentType, | ||
| }; | ||
| use opentelemetry::trace::TraceContextExt; | ||
| use thiserror::Error; | ||
|
|
@@ -178,6 +178,9 @@ pub struct KnnFilterOrchestrator { | |
| // Fetched logs | ||
| fetched_logs: Option<FetchLogOutput>, | ||
|
|
||
| // Read level for consistency vs performance tradeoff | ||
| read_level: ReadLevel, | ||
|
|
||
| // Pipelined operators | ||
| filter: Filter, | ||
|
|
||
|
|
@@ -186,6 +189,7 @@ pub struct KnnFilterOrchestrator { | |
| } | ||
|
|
||
| impl KnnFilterOrchestrator { | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn new( | ||
| blockfile_provider: BlockfileProvider, | ||
| dispatcher: ComponentHandle<Dispatcher>, | ||
|
|
@@ -194,6 +198,7 @@ impl KnnFilterOrchestrator { | |
| collection_and_segments: CollectionAndSegments, | ||
| fetch_log: FetchLogOperator, | ||
| filter: Filter, | ||
| read_level: ReadLevel, | ||
| ) -> Self { | ||
| let context = OrchestratorContext::new(dispatcher); | ||
| Self { | ||
|
|
@@ -204,10 +209,29 @@ impl KnnFilterOrchestrator { | |
| collection_and_segments, | ||
| fetch_log, | ||
| fetched_logs: None, | ||
| read_level, | ||
| filter, | ||
| result_channel: None, | ||
| } | ||
| } | ||
|
|
||
| fn create_filter_task( | ||
| &self, | ||
| logs: FetchLogOutput, | ||
| ctx: &ComponentContext<Self>, | ||
| ) -> TaskMessage { | ||
| wrap( | ||
| Box::new(self.filter.clone()), | ||
| FilterInput { | ||
| logs, | ||
| blockfile_provider: self.blockfile_provider.clone(), | ||
| metadata_segment: self.collection_and_segments.metadata_segment.clone(), | ||
| record_segment: self.collection_and_segments.record_segment.clone(), | ||
| }, | ||
| ctx.receiver(), | ||
| self.context.task_cancellation_token.clone(), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
|
|
@@ -272,14 +296,28 @@ impl Orchestrator for KnnFilterOrchestrator { | |
| Span::current().add_link(prefetch_span.context().span().span_context().clone()); | ||
| tasks.push((prefetch_metadata_task, Some(prefetch_span))); | ||
|
|
||
| // Fetch log task. | ||
| let fetch_log_task = wrap( | ||
| Box::new(self.fetch_log.clone()), | ||
| (), | ||
| ctx.receiver(), | ||
| self.context.task_cancellation_token.clone(), | ||
| ); | ||
| tasks.push((fetch_log_task, Some(Span::current()))); | ||
| match self.read_level { | ||
| ReadLevel::IndexOnly => { | ||
| // For IndexOnly queries, skip log fetching and use empty logs | ||
| tracing::info!("Skipping log fetch for IndexOnly read level"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Maintainability] Using Context for Agents |
||
| let empty_logs = FetchLogOutput::new(Vec::new().into()); | ||
| self.fetched_logs = Some(empty_logs.clone()); | ||
|
|
||
| // Immediately schedule the filter task with empty logs | ||
| let filter_task = self.create_filter_task(empty_logs, ctx); | ||
| tasks.push((filter_task, Some(Span::current()))); | ||
| } | ||
| ReadLevel::IndexAndWal => { | ||
| // Fetch log task for full consistency. | ||
| let fetch_log_task = wrap( | ||
| Box::new(self.fetch_log.clone()), | ||
| (), | ||
| ctx.receiver(), | ||
| self.context.task_cancellation_token.clone(), | ||
| ); | ||
| tasks.push((fetch_log_task, Some(Span::current()))); | ||
| } | ||
| } | ||
|
|
||
| tasks | ||
| } | ||
|
|
@@ -326,17 +364,7 @@ impl Handler<TaskResult<FetchLogOutput, FetchLogError>> for KnnFilterOrchestrato | |
|
|
||
| self.fetched_logs = Some(output.clone()); | ||
|
|
||
| let task = wrap( | ||
| Box::new(self.filter.clone()), | ||
| FilterInput { | ||
| logs: output, | ||
| blockfile_provider: self.blockfile_provider.clone(), | ||
| metadata_segment: self.collection_and_segments.metadata_segment.clone(), | ||
| record_segment: self.collection_and_segments.record_segment.clone(), | ||
| }, | ||
| ctx.receiver(), | ||
| self.context.task_cancellation_token.clone(), | ||
| ); | ||
| let task = self.create_filter_task(output, ctx); | ||
| self.send(task, ctx, Some(Span::current())).await; | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
ReadLevelfeels unclear to me, is there a use case for this other than toggling whether to read logs or notThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was "eventual_consistency" before. I went back and forth on many names. I think its the clearest one I can generate that is clear, specific, and correct in its use of terms.
Databases often offer "levels" to a read but we are devoid of any formal naming here, so I prefer to be explicit in IndexOnly vs IndexAndWal. Then the question is what to call those options. If its "ConsistencyLevel" I'd expect formal notions of consistency not wishy-washy ones like what we are offering. The reason to make it an enum is we can offer
BoundedConsistencyin the future.