Remote compaction thread count

[rban1]

Differential Revision: D118323642

[meta-codesync]

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

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 380.3s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 10d6a12

---

Summary

Solid approach to allowing remote-compaction-waiting threads to not consume compaction slots. The core mechanism (callback + counter + scheduling adjustment) is sound and the test is well-designed. A few consistency and style issues to address.

High-severity findings (1):

• [compaction_job.cc:501] AcquireSubcompactionResources computes available compaction slots without subtracting bg_remote_compaction_waiting_, inconsistent with the updated scheduling logic.

Full review (click to expand)

Findings

🔴 HIGH

H1. AcquireSubcompactionResources uses stale slot-availability formula -- compaction_job.cc:501

• Issue: AcquireSubcompactionResources computes available_bg_compactions_against_db_limit as max_db_compactions - *bg_compaction_scheduled_ - *bg_bottom_compaction_scheduled_. After this PR, MaybeScheduleFlushOrCompaction uses the adjusted formula bg_compaction_scheduled_ + bg_bottom_compaction_scheduled_ - bg_remote_compaction_waiting_. But AcquireSubcompactionResources does not subtract waiting compactions, so it will undercount available slots when remote compactions are waiting. This means round-robin subcompaction resource acquisition is more conservative than necessary when remote compactions are in flight.
• Root cause: The "available compaction slots" formula was duplicated in two places; only one was updated.
• Suggested fix: Pass bg_remote_compaction_waiting_ to CompactionJob (or access it via the same pattern as bg_compaction_scheduled_) and subtract it in AcquireSubcompactionResources line 501-503. Alternatively, extract a shared helper for the effective-active-count computation.

🟡 MEDIUM

M1. CaptureBackgroundJobPressure does not reflect waiting threads -- db_impl_compaction_flush.cc:3631

• Issue: CaptureBackgroundJobPressure() reports compaction_scheduled = bg_compaction_scheduled_ + bg_bottom_compaction_scheduled_ without subtracting bg_remote_compaction_waiting_. This means the OnBackgroundJobPressureChanged listener callback will report a higher scheduled count than the effective active count, potentially causing external thread-pool expansion logic to make suboptimal decisions.
• Suggested fix: Consider adding compaction_remote_waiting to BackgroundJobPressure so listeners can distinguish active from waiting compactions.

M2. New defaulted parameter violates CLAUDE.md guidance -- compaction_job.h:170

• Issue: CLAUDE.md explicitly warns: "Avoid new defaulted parameters. This is the Miss Spelling in README #1 trap on refactoring!" The new parameter std::function<void(bool)> remote_compaction_wait_state_changed = {} is added as a defaulted parameter at the end of the constructor's already-long parameter list.
• Suggested fix: Since the constructor already has many defaulted parameters (this is a pre-existing pattern), this is a style concern rather than a correctness issue. However, consider whether a struct/options bundle for the compaction-job configuration would be more appropriate long-term.

M3. Thread pool exhaustion is unaddressed -- db_impl_compaction_flush.cc:3580

• Issue: When remote compactions are waiting, new compactions are scheduled to the thread pool. If all LOW-priority threads are blocked in Wait(), the newly scheduled compactions will queue in the Env thread pool but cannot execute until a thread becomes available. This isn't a bug (the Env queuing handles it), but the user gets no signal that their thread pool is undersized for the configured concurrency. The test explicitly works around this by calling env_->SetBackgroundThreads(2, Env::Priority::LOW) to ensure threads are available.
• Suggested fix: Document in the compaction service documentation that users should configure max_background_compactions or thread pool size to account for remote wait parallelism. Consider whether SetBackgroundThreads should be called automatically when remote waits start.

🟢 LOW / NIT

L1. BlockingCompactionService is single-use -- compaction_service_test.cc:1939

• Issue: The released_ flag is set to true and never reset. If the service is reused (e.g., compactions triggered after Release()), subsequent Wait() calls will not block. This is fine for the current test but limits reusability.
• Suggested fix: Not actionable for this PR; the test is correct as-is.

L2. Consider UNLIKELY on the callback check -- compaction_service_job.cc:~140

• Issue: The checks if (is_primary_subcompaction && remote_compaction_wait_state_changed_) are on a cold path (once per compaction service job), so this is purely a nit.
• Suggested fix: No action needed.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
CompactionServiceCompactionJob (remote worker)	NO -- callback defaults to empty	YES	Safe
CompactFilesImpl	YES -- callback passed	YES	Correct
BackgroundCompaction	YES -- callback passed (per diff)	YES, but diff shows old code in repo	Verify diff applied
Bottom-priority compactions	YES -- same CompactionJob constructor	Callback passed in bottom-pri path	Correct
DB shutdown	YES -- callback fires before bg_compaction_scheduled_--	YES -- Defer fires before BackgroundCallCompaction cleanup	Safe
Compaction abort	Callback fires during Wait()	Defer handles cleanup	Safe
AcquireSubcompactionResources	YES	NO -- formula inconsistent	H1
CaptureBackgroundJobPressure	YES	Partial -- reports raw counts	M1

Bottom-priority compaction analysis: The diff updates the BackgroundCompaction call site at line 5063, which handles both LOW and BOTTOM priority compactions (bottom-pri goes through the BGWorkBottomCompaction -> BackgroundCallCompaction -> BackgroundCompaction path, which uses the same CompactionJob constructor at line 5049). Per the diff, the callback is passed. The bg_remote_compaction_waiting_ subtraction covers both LOW and BOTTOM scheduled counts, so bottom-pri remote compactions will correctly free a slot. However, the counter tracks "waiting" globally without distinguishing priority -- this is correct since the scheduling check also uses the combined count.

Subcompaction analysis: Only the primary subcompaction fires the callback. This is correct: a CompactionJob occupies one bg_compaction_scheduled_ slot (with optional extra slots from AcquireSubcompactionResources). Each subcompaction calls ProcessKeyValueCompactionWithCompactionService independently, but they share the same CompactionJob and its single slot. Freeing one slot when the primary enters Wait() is the right granularity.

Lifecycle analysis: The callback [this](bool waiting) { OnRemoteCompactionWaitStateChanged(waiting); } captures this (DBImpl*). The CompactionJob is stack-allocated inside BackgroundCompaction/CompactFilesImpl, and the Defer fires before the function returns. BackgroundCallCompaction decrements bg_compaction_scheduled_ after the compaction job completes, so DBImpl is still alive when the callback fires.

Positive Observations

• The "primary subcompaction" check (sub_compact == compact_->sub_compact_states.data()) is a clean, zero-overhead way to identify the first subcompaction without adding bookkeeping.
• Using Defer for the cleanup ensures the counter is balanced even if Wait() throws or returns early.
• The test design (2 CFs, max_background_compactions=1, blocking service) cleanly validates the core feature.
• The assert bg_remote_compaction_waiting_ <= bg_compaction_scheduled_ + bg_bottom_compaction_scheduled_ is a good safety net for debug builds.

---

ℹ️ 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