[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 |
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix with proper validation and test coverage. LGTM.
Automated review by github-manager-bot
Signed-off-by: Rui <1685901819@qq.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10758 +/- ##
=============================================
+ Coverage 48.29% 48.55% +0.26%
- Complexity 13502 13684 +182
=============================================
Files 1380 1381 +1
Lines 101091 101517 +426
Branches 13101 13199 +98
=============================================
+ Hits 48824 49294 +470
+ Misses 46305 46208 -97
- Partials 5962 6015 +53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 753 added lines in this PR, 629 are deterministic regression tests; the production change is limited to
ConsumeQueueandConsumeQueueExt.Cleanup State and Recovery
How Did You Test This Change?
develop: the deterministic truncate/reload regression failed in 5/5 isolated JDK 8 Maven processes.ConsumeQueueTest: 21/21, including pending cleanup after raw-tags fallback, automatic retry after mapped-buffer release, and preservation of an Ext reference before the three-file recovery window.store -am test: common 241/241, remoting 174/174, and store 324 tests with 4 skips, 0 failures, and 0 errors.git diff --check: passed.