-
Notifications
You must be signed in to change notification settings - Fork 43
Stream: Store offset only for first message per segment #1479
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
…es following messages based on that. saves us from having to write "x-stream-offset":i64 for each message
|
Claude finished @viktorerlingsson's task —— View job Code Review Progress
Issues FoundPotential race condition in consumer offset handling (stream_message_store.cr:368-377): Missing error handling in offset calculation (stream_message_store.cr:102): Inconsistent state handling (stream_message_store.cr:295): |
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
… @segment_first_ts for clarity
| msg = BytesMessage.from_bytes(mfile.to_slice + pos) | ||
| offset = offset_from_headers(msg.properties.headers) | ||
| offset = @segment_first_offset[seg] | ||
| mfile.pos = 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.
This is not introduced, or only a problem here, but using the internal MFile pos wont be thread safe. Doing a .dup of the MFile is one way to solve it, there are probably more efficent ways.
WHAT is this pull request doing?
Optimize stream message storage by only storing offsets for the first message in each segment instead of every message. The offset for subsequent messages is calculated on-demand based on the segment's first offset and message position in the segment.
x-stream-offsetheader from messages at write time, reducing disk I/O and message sizeIncreases write-performance by around ~30% with small messages in my tests. Doesn't have a big impact on read performance.
Works with streams created before this change as well, but messages published with the changes in this PR can not be read by older lavinmq versions.
HOW can this pull request be tested?
Should be well covered by existing specs.