[ENH] Make SysDb return eligible collections to GC. #3916
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
| // before this cutoff time. | ||
| // SysDb can apply additional logic to return the collections that should | ||
| // be prioritized for GC. | ||
| optional uint64 cutoff_time_secs = 1; |
There was a problem hiding this comment.
I think the sysdb should fail requests with a cutoff time of less than 30 mins from NOW() or similar as a backstop/defensive measure. Or cutoff times in the future etc.
There was a problem hiding this comment.
Thats a good suggestion. Let me send a separate PR or make it part of another PR where i am doing some cleanup on the rust side.
Tracking it here - https://github.com/chroma-core/hosted-chroma/issues/1791
| // be prioritized for GC. | ||
| optional uint64 cutoff_time_secs = 1; | ||
|
|
||
| // Limit the number of collections that can be returned. |
There was a problem hiding this comment.
Are you assuming one GC node is sufficient?
There was a problem hiding this comment.
The purpose of limit is to make GC do smaller runs so that in case of midway crash it does not restart the whole thing again.
Assuming more than one node will be needed.
Either node can use limit (without any offset).
They will both get the same n collections. Will use rendezvous hash to determine if they should do it.
After a run to GC these n collections, it will request n more.
[We could choose to use same cutoff (preferred) or new cutoff in the next run. Still need to think through that.]
| @@ -835,7 +835,10 @@ impl GrpcSysDb { | |||
| ) -> Result<Vec<CollectionToGcInfo>, GetCollectionsToGcError> { | |||
| let res = self | |||
| .client | |||
| .list_collections_to_gc(chroma_proto::ListCollectionsToGcRequest {}) | |||
| .list_collections_to_gc(chroma_proto::ListCollectionsToGcRequest { | |||
| cutoff_time_secs: None, | |||
There was a problem hiding this comment.
should we pipe these values through?
There was a problem hiding this comment.
This PR does not contain the rust side changes. I only added this for the code to compile. Sending another PR for using these correctly on the rust side.
| @@ -0,0 +1,2 @@ | |||
| -- Modify "collections" table | |||
| ALTER TABLE "public"."collections" ADD COLUMN "num_versions" bigint NULL DEFAULT 0, ADD COLUMN "oldest_version_ts" timestamp NULL; | |||
There was a problem hiding this comment.
nit: integer should suffice over bigint?
There was a problem hiding this comment.
Limit the number of collections, and return those that have at least 1 version that is elligible for GC.
|
@HammadB - removed the migration from this PR, and added test cases. Can add few more tests before merging. |
Limit the number of collections, and return
those that have at least 1 version that is
elligible for GC.
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?