KAFKA-14491: [17/N] Refactor segments cleanup logic#13365
Merged
Conversation
mjsax
approved these changes
Mar 14, 2023
mjsax
left a comment
mjsax
left a comment
Member
There was a problem hiding this comment.
Overall LGTM. One nit question.
|
|
||
| @Override | ||
| public void cleanupExpiredSegments(final long streamTime) { | ||
| super.cleanupExpiredSegments(streamTime); |
Member
There was a problem hiding this comment.
If we only call super... why do we need to overwrite the method to begin with?
Contributor
Author
There was a problem hiding this comment.
The method from AbstractSegments is protected, but LogicalKeyValueSegments needs to expose it publicly so that it can be called from the versioned store implementation.
Admittedly looks odd at first glance. I can add a comment into the code if you think it'd be useful?
Member
There was a problem hiding this comment.
Ah. I see. Guess it would get clear quickly in an IDE -- just hard to see on GitHub. Seems fine w/o a comment.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Prior to this PR, AbstractSegments automatically called the helper method to clean up expired segments as part of
getOrCreateSegmentIfLive(). This works fine for windowed store implementations which callgetOrCreateSegmentIfLive()exactly once perput()call, but is inefficient and difficult to reason about for the new RocksDBVersionedStore implementation (cf. #13188) which makes potentially multiple calls togetOrCreateSegmentIfLive()for different segments for a singleput()call. This PR addresses this by refactoring the call to clean up expired segments out ofgetOrCreateSegmentIfLive(), opting to have the different segments implementations specify when cleanup should occur instead. After this PR, RocksDBVersionedStore only cleans up expired segments once per call toput().Committer Checklist (excluded from commit message)