[ISSUE #10755] Fix ConsumeQueueExt truncation cleanup - #10758
Conversation
Signed-off-by: Rui <1685901819@qq.com>
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes ConsumeQueueExt data orphaning during truncation and recovery by refactoring truncateDirtyLogicFiles, adding truncateAll() to ConsumeQueueExt, and handling the empty-mapped-files recovery path.
Findings
- [Warning]
ConsumeQueueExt.java:232— ThetruncateAllPendingvolatile flag is set totrueduringtruncateAll()but is never reset tofalse. After recovery truncation completes, all subsequentput()calls will be silently skipped with a warning log. If the ConsumeQueue is expected to accept new ext writes after recovery, this flag should be reset once truncation finishes. - [Info]
ConsumeQueue.java:448-492— The refactored truncation loop is significantly cleaner. ThehasRetainedExt/cqFileDeletionFailedtracking properly handles edge cases where file deletion fails mid-truncation. - [Info]
ConsumeQueue.java:136-147— Good catch on the empty mapped files recovery path — previously this case would skip ext cleanup entirely. - [Info]
findLastRetainedExtAddress()— The backward scan is correct but could be expensive for large queues. This is only called during truncation so the impact should be limited.
Suggestions
- Important: Verify whether
truncateAllPendingshould be reset aftertruncateAll()completes. If the ConsumeQueue continues to serve writes post-recovery, the current behavior would silently drop all ext data. - Consider logging at INFO level (not WARN) when
truncateAllPendingblocks a write during active truncation, since this is expected transient behavior.
Automated review by github-manager-bot
|
Clarification on the automated review warning: |
|
Thanks for the clarification, @ai-yang. You're right — I can see that |
Which Issue(s) This PR Fixes
Fixes #10755
Brief Description
ConsumeQueue.truncateDirtyLogicFiles()truncated the main consume queue but did not trimConsumeQueueExtto the last retained extension address. Ext units from the discarded CQ tail therefore remained readable, and later appends plus reload/recovery could preserve those orphaned units permanently.This change aligns CQExt truncation with the successfully retained CQ state:
minLogicOffsetand truncate to it;deleteFile=falsedo not mutate CQExt independently;Of the 607 added lines in this PR, 494 are deterministic regression tests; the production change is limited to
ConsumeQueueandConsumeQueueExt.How Did You Test This Change?
develop: the deterministic truncate/reload regression failed in 5/5 isolated JDK 8 Maven processes.minLogicOffset, and recovery.store -am test: common 241/241, remoting 174/174, and store 322 tests with 4 skips, 0 failures, and 0 errors.git diff --check: passed.