[ISSUE #10755] Fix ConsumeQueueExt truncation cleanup - #10758
Conversation
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10758 +/- ##
=============================================
- Coverage 48.58% 48.54% -0.04%
- Complexity 13676 13677 +1
=============================================
Files 1381 1381
Lines 101475 101528 +53
Branches 13190 13202 +12
=============================================
- Hits 49299 49285 -14
- Misses 46174 46224 +50
- Partials 6002 6019 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed after new commits — ConsumeQueueExt truncation now handles the empty main CQ edge case and refactors truncation into a shared method.
Findings
- [Info]
recover()now correctly callsconsumeQueueExt.recover()andtruncateConsumeQueueExt(1)when the main CQ has no mapped files — previously this path was skipped entirely, leaving ext files in an inconsistent state. - [Info]
truncateConsumeQueueExt(long)extracted from inline logic, reused by bothrecover()andtruncateDirtyLogicFiles(). - [Info]
truncateDirtyLogicFilesrefactored:shouldDeleteFilescoped to the inner loop,cqFileDeletionFailedtracks outer state — clearer intent. - [Info] Test covers the empty CQ + ext recovery scenario.
LGTM.
Automated review by github-manager
Signed-off-by: Rui <1685901819@qq.com>
82bc882 to
00e1c97
Compare
|
Update after refreshing this PR against the latest
The full CI matrix has been retriggered by the force-push. @RongtongJin @guyinyou, could one of you please take a human review when available, particularly for the CQ/CQExt truncation and recovery invariants? Thank you. |
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.