[ENH] Update compactor to flush total records upon compaction #3483
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
drewkim
force-pushed
the
drew/_enh_update_compactor_to_flush_total_records_upon_compaction
branch
from
b0e05a7 to
6b32b1b
Compare
| @@ -133,12 +133,23 @@ impl ArrowUnorderedBlockfileWriter { | |||
| Box::new(ArrowBlockfileError::MigrationError(e)) as Box<dyn ChromaError> | |||
| })?; | |||
|
|
|||
| let total_keys = self | |||
There was a problem hiding this comment.
We already loop over the blocks above, do we need a second iteration?
There was a problem hiding this comment.
Is the suggestion to calculate the total before the loop and then iteratively add to the total for each delta?
| @@ -733,6 +744,7 @@ mod tests { | |||
| writer.set(prefix_2, key2, value2).await.unwrap(); | |||
|
|
|||
| let flusher = writer.commit::<&str, Vec<u32>>().await.unwrap(); | |||
| assert_eq!(Some(2_u64), flusher.total_keys()); | |||
There was a problem hiding this comment.
We need more testing around this.
There was a problem hiding this comment.
Agreed, wanted to make sure I wasn't missing the mark directionally before going deeper. Will write next
| LogPosition int64 | ||
| CurrentCollectionVersion int32 | ||
| FlushSegmentCompactions []*FlushSegmentCompaction | ||
| TotalRecordsPostCompaction uint64 |
There was a problem hiding this comment.
Will you have to backfill, or is the frontend expected to handle null?
There was a problem hiding this comment.
I don't think we need to backfill as the field defaults to 0 and over time will become correct as collections are updated. The potential overage is bounded by compaction frequency. Happy to discuss here
|
Only one main comment: update the record count together with the log offset and collection version in the same query instead of doing two queries to DB |
drewkim
force-pushed
the
drew/_enh_update_compactor_to_flush_total_records_upon_compaction
branch
from
b794d14 to
25c1fb0
Compare
drewkim
changed the base branch from
drew/_enh_add_num_records_last_compaction_to_sysdb
to
main
Description of changes
Updates the compactor to calculate the total records per collection and flush to the sysdb upon every compaction.
Summarize the changes made by this PR.
FlushCollectionCompactionstruct includesTotalRecordsPostCompactionSysDBpopulates thetotal_records_post_compactioncolumn when receiving a flushArrowBlockfileFlushercontains a new attribute,total_keysArrowUnorderedBlockfileWritersums the total keys using theSparseIndexWriterand returns anArrowBlockfileFlusherwith the summed countRegisterInputcontains a new attribute,total_records_post_compactionCompactOrchestrator, when handlingCommitSegmentWriterOutput, receives aChromaSegmentFlusher::RecordSegment, it readstotal_keys()and sets it as an attribute on itself.ChromaSegmentFlusher::RecordSegmenthastotal_keys()through itsArrowBlockfileFlusherCompactOrchestratorsends itsnum_records_last_compactionvalue to aRegisterInputto be flushed to theSysDBTest plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustSysDBDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?