-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ENH] get collection info for compactor #1778
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
// protoc-gen-go v1.26.0 | ||
// protoc v4.24.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.
I will fix this
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.
LGTM
log.Error("error getting collection info", zap.Error(err)) | ||
return nil, grpcutils.BuildInternalGrpcError("error getting collection info") | ||
} | ||
for index := range recordLogs { |
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.
for _, recordLog := range recordLogs {
}
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.
sure
select r.collection_id, r.id, r.timestamp, row_number() over(partition by r.collection_id order by r.id) as rank | ||
from record_logs r, collections c | ||
where r.collection_id = c.id | ||
and r.id>c.log_position |
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.
Will compactor update the log position?
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.
yes go API is tracked here https://linear.app/trychroma/issue/CHR-298/segment-compaction-flush
log position needs to be updated in the same transaction that also registers S3 files.
5d417b4
to
516b33b
Compare
## Description of changes https://linear.app/trychroma/issue/CHR-293/get-collection-ids-for-compactor - get collection information for collections that need to be compacted, order by timestamps of the log - DB retry is not included ## Test plan - [ ] record_log_test and record_log_service_test
Description of changes
https://linear.app/trychroma/issue/CHR-293/get-collection-ids-for-compactor
Test plan
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?