-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ENH] Make a DirtyMarker have a backfill bit. #5850
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
base: main
Are you sure you want to change the base?
Conversation
We want the ability to run a task for the first time on a collection. Backfills coalesce.
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Add This PR introduces a Key Changes• Added Affected Areas• Protobuf contract (DirtyMarker message) This summary was automatically generated by @propel-code-bot |
| // If the request was the result of the backfill call---operator's intention left to the spurr of | ||
| // the moment. | ||
| bool backfill = 4; |
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.
[Documentation]
This comment is a bit unclear and informal. A more descriptive comment would improve clarity for future developers. Also, there's a typo in "spurr".
Context for Agents
[**Documentation**]
This comment is a bit unclear and informal. A more descriptive comment would improve clarity for future developers. Also, there's a typo in "spurr".
File: idl/chromadb/proto/logservice.proto
Line: 72There 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.
I want it murky so we can always repurpose backfill once we do this one.
This comment has been minimized.
This comment has been minimized.
|
|
||
| message BackfillRequest { | ||
| string collection_id = 1; | ||
| uint64 initial_insertion_epoch_us = 2; |
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.
The backfill will only trigger at this point.
rust/log-service/src/lib.rs
Outdated
| .map_err(|_| Status::invalid_argument("Failed to parse collection id"))?; | ||
| tracing::info!( | ||
| "backfill for {collection_id} at {}", | ||
| request.initial_insertion_epoch_us |
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.
I believe this initial_insertion_epoch_us should be chosen to be some time in the past over here instead of being passed in. The RLS would know better about how far in the past to go than the caller.
Description of changes
We want the ability to run a task for the first time on a collection.
Backfills coalesce.
Test plan
New tests added, so CI.
Migration plan
Backwards-compatible serialization types due to proto.
Observability plan
N/A
Documentation Changes
N/A