Repository navigation
[ISSUE #11439] fix(store): start compaction rounds from an empty scratch folder - #11440
iloveeyjafjalla wants to merge 1 commit into
Conversation
…y scratch folder
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes a data corruption issue in the compaction layer where an interrupted compaction round leaves stale files in the scratch folder. The next round reuses the same file name without truncation, causing stale data from the aborted round to be mixed into the live compaction log and consume queue.
Findings
- [Info]
TopicPartitionLog.java:682-685— The newcleanScratchFoldermethod correctly deletes both the store and CQ directories before a compaction round starts. This ensures that any leftover files from an interrupted round are cleaned up before the new round begins. - [Info]
TopicPartitionLog.java:202— The fix is also applied topullMessageFromMasterwhich usesREPLICATING_SUB_FOLDER, ensuring consistency across both scratch-folder patterns. - [Info]
CompactionLogTest.java— The regression test pre-creates a leftover scratch file and verifies it is cleaned up, which directly validates the fix.
Suggestions
- The fix is minimal and focused on the root cause. The approach of cleaning the scratch folder before starting a new round is correct and safe, as the scratch state of an aborted round is inherently unreliable and should be discarded.
- Consider adding a brief comment in
cleanScratchFolderexplaining why this is necessary (i.e., thatclean(...)only walks the in-memory file list, which is empty for a freshly constructed scratch log). This will help future maintainers understand the intent.
Overall
The fix addresses a subtle but serious data corruption issue. The implementation is clean, the test coverage is good, and the change is minimal. LGTM.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes data corruption in compaction when a round is interrupted: the scratch folder is now cleaned before each round starts, preventing stale bytes from a previous aborted round from being carried into the live log.
Findings
- [Info] CompactionLog.java:682-691 — is a clean static helper. Called at the right points (both and ).
- [Info] CompactionLogTest.java — Test pre-creates the leftover file and verifies cleanup. Good reproduction of the interrupted-round scenario.
Verdict
LGTM. Important data integrity fix with clear root cause analysis.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Brief Description
A compaction round writes its output into a scratch sub-folder and moves it into the live compaction log only when the round completes:
TopicPartitionLog.clean(...)walks the in-memory file list, which is empty for a freshly constructed scratch log, so files left on disk by an interrupted round are invisible to it — and nothing else ever cleans them. The next round then reuses the very same file name (MappedFileQueue.tryCreateMappedFileopens the existing…/compacting/00000000000000000000throughDefaultMappedFile, which maps itrwwithout truncating and withwrotePosition = 0), overwrites it from offset 0, declares it full viaputEndMessage, andreplaceFiles/replaceCqFilesmove it into the live compaction log and consume queue — with whatever the aborted round had written beyond the new content still in it. The compacted view then mixes records of two rounds (stale/duplicated entries and a consume-queue tail that does not belong to the log content).This change empties the scratch folder before a round starts, so the scratch state of an aborted round can at worst be lost (the messages are re-compacted by the next round) instead of being spliced into the live log:
It is called for both scratch folders that follow this pattern:
COMPACTING_SUB_FOLDERincompaction(...)andREPLICATING_SUB_FOLDERinpullMessageFromMaster(...)(:202). Clean rounds are unaffected (the folders are empty then) and the existing post-roundclean(...)check keeps working.How Did You Test This Change?
New test
CompactionLogTest#testCompactionCleansTheScratchFolderOfAnInterruptedRound: it pre-creates the leftover scratch file…/<topic>/<queueId>/compacting/00000000000000000000(as an interrupted round leaves it), runs the realcompaction(...)through the existing mock harness withputEndMessagestubbed, and asserts the leftover is gone afterwards.Results on Temurin JDK 8 (the JDK CI uses), base
0dd48c39dplus this patch:java.lang.AssertionError: a scratch file left by an interrupted round must not survive into the next roundCompactionLogTest—Tests run: 5, Failures: 0, Errors: 0, Skipped: 1(the skip is the pre-existingtestReplaceFiles, which isAssumed off on Windows)mvn -B -ntp -pl store package -Dtest='CompactionLogTest,CompactionPositionMgrTest,OffsetMapTest':BUILD SUCCESS, 9 tests / 0 failures,You have 0 Checkstyle violations.,BugInstance size is 0Disclosure: the whole
storesuite cannot be run on this machine (it crashes inside native RocksDB after roughly 45 test classes — unrelated to this change, and every affected class passes on its own), so I report the compaction-related classes plus the static checks rather than a full-module claim. The change touches file-based compaction code only and does not involve RocksDB.This change was prepared with AI assistance (analysis, patch and test drafting); the diff and every result above were reproduced locally by running the module's tests against the base commit and the patched tree.