KAFKA-19164: Keep track of groups when deleting transactional offsets #8
Conversation
When deleting pending transactional offsets, we must preserve the list of groups associated with the producer ID, otherwise we cannot clean up the list of pending transactions for the group once the transaction is committed or aborted.
WalkthroughThe changes introduce a new boolean parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OffsetMetadataManager
participant Offsets
participant Group
Client->>OffsetMetadataManager: Commit offset (with/without producer id)
OffsetMetadataManager->>Offsets: Add/update offset (preserveGroups depends on context)
Client->>OffsetMetadataManager: Delete offset (possibly pending transactional)
OffsetMetadataManager->>Offsets: Remove offset
Offsets-->>OffsetMetadataManager: Conditionally remove group entry (based on preserveGroups)
OffsetMetadataManager->>Group: Check if group can be deleted
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a mechanism to preserve group information when deleting transactional offsets, which is crucial for cleaning up pending transactions. The changes seem well-structured and include a new test case to validate the cleanup behavior. Overall, the changes look good, but I have a few suggestions for improvement.
Summary of Findings
- Clarity of 'preserveGroups' usage: The purpose of the
preserveGroupsflag could be made more explicit in theOffsetsclass constructor to improve readability. - Test case description: The test case
testCleanupExpiredOffsetsWithDeletedPendingTransactionalOffsetscould benefit from a more descriptive name or additional comments to clarify its purpose.
Merge Readiness
The changes appear to be well-implemented and include a new test case. However, addressing the comments regarding the clarity of preserveGroups and the test case description would further improve the quality of the code. I am unable to directly approve the pull request, and recommend that others review and approve this code before merging. I recommend that the pull request not be merged until the high severity issues are addressed (at a minimum).
When deleting pending transactional offsets, we must preserve the list
of groups associated with the producer ID, otherwise we cannot clean up
the list of pending transactions for the group once the transaction is
committed or aborted.
Summary by CodeRabbit
Bug Fixes
Tests