[omkarhgawde]

Summary:
Why the change is required:
When a resumable remote compaction (OpenAndCompactOptions::allow_resumption) resumes, MaybeResumeSubcompactionProgressOnInputIterator() restores the saved output files of the output level and, with per-key placement, of the proximal output level. It reads their table properties with CompactionJob::ReadOutputFilesTableProperties(). With per-key placement, a debug build aborted on resume when one of the two levels had no output files, and a failure on a proximal level file was logged as an output level failure.

Fix: allow a level with no output files. A level can be empty when the progress is saved, for example when every key so far goes to the last level. ReadOutputFilesTableProperties() then failed assert(!output_files.empty()). Release builds compile the assert out, and the rest of the restore path already handles an empty list: there are no properties to read, no files or output records to restore, and no file numbers to skip. The assert is removed, with a comment that the list can be empty.

Fix: label the level correctly. level_type was a function-level static, so the first call in the process fixed the label for every later call. The only caller also did not pass is_proximal_level, so it always took its default of false. The caller's own log printed "output levelfiles", with a missing space. level_type is now a normal local variable, the caller passes is_proximal_level, and the parameter default is removed so that a later caller cannot omit it.

Safety and limitations:
Release behavior does not change, except for the log and status text. The empty-level case needs per-key placement (the compaction outputs to the last level and preclude_last_level_data_seconds is set) and saved progress with files at only one of the two levels. Progress is not saved while per-key placement writes blob files. A related case, where the other level still has an open file when the progress is saved, is fixed separately in D128238210.

Differential Revision: D128228840

[meta-codesync]

@omkarhgawde has exported this pull request. If you are a Meta employee, you can view the originating Diff in D128228840.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit ccdae60

---

Summary

Clean, well-scoped bugfix that corrects two real defects in resumable remote compaction: (1) a debug-only assertion failure when one output level is empty, and (2) a static local variable that permanently latched the wrong level label. Both fixes are correct and the downstream code already handles empty output files safely. The test directly exercises the triggering scenario.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Status::Corruption in caller error path does not include level label — compaction_job.cc:3568

• Issue: When ReadOutputFilesTableProperties() fails, the Status::Corruption message returned at line 3568-3570 does not include which level failed ("output" vs "proximal output"). The ROCKS_LOG_ERROR now correctly labels the level, but the returned Status still says only "Not able to resume due to table property reading error". A caller receiving this Status cannot distinguish which level had the error without checking the log.
• Root cause: The Status::Corruption message was not updated alongside the log message fix.
• Suggested fix: Include the level label in the Corruption message. Alternatively, since ReadOutputFilesTableProperties() already embeds level_type in its own error Status messages (lines 3398-3400, 3410-3412), the inner Status s already carries the level information via s.ToString() which is appended. This may be sufficient — worth verifying the inner status string is meaningful enough for diagnosis.

🟢 LOW / NIT

L1. No test for the reverse empty-level case — compaction_service_test.cc

• Issue: The new test covers empty proximal level (all keys go to last level). The symmetric case — empty output level with all keys going to the proximal level — is not tested. While harder to construct, it's a valid scenario the comment in the fix acknowledges ("one of the two output levels may have no output file yet").
• Suggested fix: Consider adding a test where preclude_last_level_data_seconds is set high enough that all keys go to the proximal level, leaving the output level empty.

L2. Minor log redundancy — compaction_job.cc:3562

• Issue: The caller's error log now produces "for output level output files" (when is_proximal_level=false), which reads slightly redundantly. Cosmetic only.

Cross-Component Analysis

Context	Affected?	Safe?	Notes
Per-key placement	Yes	Yes	Triggering context; fix correctly handles it
WritePreparedTxnDB	No	N/A	Resume doesn't interact with transaction visibility
ReadOnly DB	No	N/A	No compaction in read-only mode
DB mutex	No	N/A	ReadOutputFilesTableProperties does file I/O, not under DB mutex
Concurrent writers	No	N/A	Resume happens before compaction iteration starts

Assumption stress-test:

• "empty output_files is safe": Verified all downstream consumers — reserve(0) is harmless, RestoreCompactionOutputs loop is a no-op, file number skip loop is a no-op. No counterexample found.
• "early-return guard doesn't prevent reaching the empty case": Confirmed — guard triggers only when BOTH levels have 0 records, but in the empty-proximal scenario the output level has records > 0.

Positive Observations

• Correct identification of the static local variable bug — a subtle C++ pitfall where the value is set only on the first call
• Removal of default parameter follows CLAUDE.md guidance and prevents future omission bugs
• Well-constructed test using sync points to force the exact scenario
• Minimal, focused scope — changes only what's necessary

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase