[v23.1.x] s/segment: fixed tracking offsets of empty segments #11543
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a segment is rolled due to the `segment.ms` property its base offset is set to the committed offset of last segment plus one. In a segment constructor all segment offset tracker offsets were set to the base offset passed as the constructor argument. This way an empty segment had exactly the same set of offsets as the segment containing single batch containing one record and base offset equal to the segment base offset. Incorrect offset accounting in empty segments lead to the situation in which eviction driven `log::truncate_prefix` was called with an incorrect offset, leading to the situation in which a log wasn't truncated at the segment boundary. This in the end resulted in the disconnection between the offset translator truncation point and first log segment start offset. For example: (we represent segment as `[base_offset,end_offset]`) In the case of log with segments: ``` [0,10][11,15] ``` After segment ms a new segment is created like this: ``` [0,10][11,15][16,16] ``` When eviction point is established the last offset is checked in this case if all the segments are to be evicted it will be equal to 16. `16` is a last offset that is going to be evicted (last included in raft snapshot). Hence the `log::truncate_prefix` is called with offset `17`. If some batches are appended to the rolled segment then the log will contain f.e. ``` [0,10][11,15][16,25] ``` Now if the log is prefix truncated at offset `17` it starts offset is updated to `17` but the underlying segment is kept. ``` [16,25] ``` Now when the reader start reading the log it will request to start from the log start offset which is `17` but it will have to skip over the batch starting at `16`. If a batch at `16` has more than one record it will still be returned to the reader and it will have to translate its base offset to kafka offset space. This is impossible as the offset_translator was already truncated to `16`. i.e. there is no information in the translator to correctly translate the offset. A fix is setting the empty segment dirty, committed and stable offset to its base_offset minus one. This way all the semantics of log operation holds as the offset returned is equal to previous segment last offset which would be the case if there would be no empty segment at the head of the log. Fixes: redpanda-data#11403 Signed-off-by: Michal Maslanka <[email protected]> (cherry picked from commit 76a9e6f)
Validating offsets in the compaction test uses committed offset but it didn't flush the log before validation. Signed-off-by: Michal Maslanka <[email protected]> (cherry picked from commit ee3843b)
Signed-off-by: Michal Maslanka <[email protected]> (cherry picked from commit a9ec0ba)
BenPope
approved these changes
Jun 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of PR #11450
Fixes: #11542,