iloveeyjafjalla opened a new pull request, #11440:
URL: https://github.com/apache/rocketmq/pull/11440
### Which Issue(s) This PR Fixes
- Fixes #11439
### 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:
```java
compacting = new TopicPartitionLog(this, COMPACTING_SUB_FOLDER); // :682 —
paths only, no load, no cleanup
...
replaceFiles(compactFiles.toCompactFiles, current, compacting); // :618 —
moves the files out
compacting.clean(false, false); // :621 —
only on the success path
```
`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.tryCreateMappedFile` opens the existing
`…/compacting/00000000000000000000` through `DefaultMappedFile`, which maps it
`rw` **without truncating** and with `wrotePosition = 0`), overwrites it from
offset 0, declares it full via `putEndMessage`, and `replaceFiles` /
`replaceCqFiles` move 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:
```java
protected static void cleanScratchFolder(TopicPartitionLog scratch) {
UtilAll.deleteFile(new File(scratch.getLog().getStorePath()));
UtilAll.deleteFile(new
File(scratch.getCQ().getMappedFileQueue().getStorePath()));
}
```
It is called for both scratch folders that follow this pattern:
`COMPACTING_SUB_FOLDER` in `compaction(...)` and `REPLICATING_SUB_FOLDER` in
`pullMessageFromMaster(...)` (`:202`). Clean rounds are unaffected (the folders
are empty then) and the existing post-round `clean(...)` 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 real `compaction(...)` through the existing mock harness
with `putEndMessage` stubbed, and asserts the leftover is gone afterwards.
Results on Temurin JDK 8 (the JDK CI uses), base `0dd48c39d` plus this patch:
- before the fix: `java.lang.AssertionError: a scratch file left by an
interrupted round must not survive into the next round`
- after the fix: `CompactionLogTest` — `Tests run: 5, Failures: 0, Errors:
0, Skipped: 1` (the skip is the pre-existing `testReplaceFiles`, which is
`Assume`d 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 0`
Disclosure: the whole `store` suite 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
