-
Notifications
You must be signed in to change notification settings - Fork 14.5k
KAFKA-17026: Implement updateCacheAndOffsets functionality on LSO movement #16459
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
Conversation
4a844ef
to
a11e394
Compare
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.
LGTM overall, some minor comments.
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.
LGTM! Just minor suggestions.
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.
lgtm
…ts in SharePartitionTest
815c7f7
to
d225e92
Compare
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.
@adixitconfluent Thanks for the PR. LGTM
…ement (apache#16459) Implemented the functionality which takes care of archiving the records when LSO moves past them. Implemented the following functions - 1. updateCacheAndOffsets - Updates the cached state, start and end offsets of the share partition as per the new log start offset. The method is called when the log start offset is moved for the share partition. 2. archiveAvailableRecordsOnLsoMovement - This function archives all the available records when they are behind the LSO. 3. archivePerOffsetBatchRecords - It archives all the available records in the per offset tracked batch passed to this function. 4. archiveCompleteBatch - It archives all the available records of the complete batch passed to this function. Reviewers: Andrew Schofield <[email protected]>,Apoorv Mittal <[email protected]>, Manikumar Reddy <[email protected]>
…ement (apache#16459) Implemented the functionality which takes care of archiving the records when LSO moves past them. Implemented the following functions - 1. updateCacheAndOffsets - Updates the cached state, start and end offsets of the share partition as per the new log start offset. The method is called when the log start offset is moved for the share partition. 2. archiveAvailableRecordsOnLsoMovement - This function archives all the available records when they are behind the LSO. 3. archivePerOffsetBatchRecords - It archives all the available records in the per offset tracked batch passed to this function. 4. archiveCompleteBatch - It archives all the available records of the complete batch passed to this function. Reviewers: Andrew Schofield <[email protected]>,Apoorv Mittal <[email protected]>, Manikumar Reddy <[email protected]>
About
Implemented the functionality which takes care of archiving the records when LSO moves past them. Implemented the following functions -
updateCacheAndOffsets
- Updates the cached state, start and end offsets of the share partition as per the new log start offset. The method is called when the log start offset is moved for the share partition.archiveAvailableRecordsOnLsoMovement
- This function archives all the available records when they are behind the LSO.archivePerOffsetBatchRecords
- It archives all the available records in the per offset tracked batch passed to this function.archiveCompleteBatch
- It archives all the available records of the complete batch passed to this function.Testing
The added functionality has been tested with unit tests.
Suppression
In order for build to pass, I had to add JavaNCSS suppression for
SharePartitionTest
. It takes into account the total lines of non-commented source code. Agreed, that we can optimize the currentSharePartitionTest
code at a few places, however, when we add new test cases, we will again breach this limit which is currently set at 1500. Hence, adding the suppression made sense to me.