Skip to content

[ISSUE #11439] fix(store): start compaction rounds from an empty scratch folder - #11440

Open
iloveeyjafjalla wants to merge 1 commit into
apache:developfrom
iloveeyjafjalla:codex/compaction-clean-subfolder
Open

iloveeyjafjalla wants to merge 1 commit into
apache:developfrom
iloveeyjafjalla:codex/compaction-clean-subfolder

Conversation

@iloveeyjafjalla

Copy link
Copy Markdown
Contributor

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:

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:

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 Assumed 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.

Copilot AI balanced review requested due to automatic review settings October 14, 2026 11:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 new cleanScratchFolder method 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 to pullMessageFromMaster which uses REPLICATING_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 cleanScratchFolder explaining why this is necessary (i.e., that clean(...) 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 RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] A compaction round interrupted in the scratch folder splices stale bytes into the live compaction log of the next round

3 participants