-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[CLN] Refactor log materializer to not need to pass in offset id explicitly for readers #2354
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
@@ -113,7 +113,7 @@ impl CompactionManager { | |||
dispatcher.clone(), | |||
None, | |||
None, | |||
Arc::new(AtomicU32::new(1)), |
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.
is this correct?
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. This is the default value that we are passing in case the segment is uninitialized. Previously we were passing the next_offset_id (as opposed to curr_max_offset_id). I changed that to pass curr_max_offset_id now.
let materializer = | ||
LogMaterializer::new(record_segment_reader, input.filtered_log.clone(), offset_id); | ||
LogMaterializer::new(record_segment_reader, input.filtered_log.clone(), None); |
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.
Why were we loading it before?
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.
We still need to read the curr_max_offset_id from the record segment. For readers, the onus of doing that has shifted to the materializer now. And writers need to still pass it explicitly since all the writer threads need to share the same curr_max_offset_id.
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 think this is fine but I skimmed it.
d088e99
to
2f76cb8
Compare
2f76cb8
to
c4199a4
Compare
…icitly for readers (chroma-core#2354) ## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes Readers can pass None for the current offset id when materializing and the materializer takes care of using the correct value by reading from the record segment. Writers still need to pass it in since it is shared by writers of different partitions. ## Test plan - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes None
Description of changes
Summarize the changes made by this PR.
Readers can pass None for the current offset id when materializing and the materializer takes care of using the correct value by reading from the record segment. Writers still need to pass it in since it is shared by writers of different partitions.
Test plan
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
None