Observability for the WriteBufferManager (#15162)

[rban1]

Summary:

Today a WriteBufferManager is nearly invisible. Its own state (memory_usage(), mutable_memtable_memory_usage(), buffer_size(), IsStallActive()) is exported nowhere -- no DB property, no ticker, no LOG line except a single message emitted when an in-line flush actually fires. Time spent blocked on the WBM limit is likewise unmeasured: STALL_MICROS, rocksdb.db.write.stall, and db.user_write_stall_micros all come from DelayWrite(), which only covers WriteController (CF-scope) stalls -- see the existing TODO on kIntStatsWriteStallMicros. And because every existing memtable counter is per-DB, a manager shared across several DBs cannot be observed as a single entity at all.

This diff adds the missing instrumentation.

Four DB properties reporting the shared manager's totals, so one DB can report for the whole manager:

• rocksdb.write-buffer-manager-memory-usage -- all accounted memory, including memtables pinned by in-flight flushes
• rocksdb.write-buffer-manager-mutable-memory-usage -- the mutable portion; the gap between the two is flush-in-flight memory
• rocksdb.write-buffer-manager-buffer-size -- the budget (flush at 7/8, stall at 1/1)
• rocksdb.write-buffer-manager-stall-active

They follow the rocksdb.block-cache-* pattern exactly (GetWriteBufferManagerForStats() mirrors GetBlockCacheForStats()), and return false -- i.e. the property is unavailable -- when no WriteBufferManager is configured.

Stall duration is now measured in WriteBufferManagerStallWrites() and recorded two ways: a new WRITE_BUFFER_MANAGER_STALL_MICROS ticker (rocksdb.write_buffer_manager.stall.micros) and a new kIntStatsWriteBufferManagerStallMicros DB stat (db.write_buffer_manager_stall_micros). The InternalStats copy matters because it lands in rocksdb.dbstats and the periodic LOG dump, so it is visible without a Statistics object -- which is the common case, since the fb_rocksdb wrapper does not export tickers unless --rocksdb_fb303_enable_stats=true. It is deliberately kept separate from STALL_MICROS so WBM and WriteController stalls stay individually attributable.

Finally, the cross-DB flush path added in D112396106 now logs which column family it picked, matching the log line the in-line path already had.

Note on Java: the TickerType byte enum is exhausted (FILE_SUBMIT_ASYNC_READ_FALLBACK occupies -0x80, the last slot, and portal.h carries a TODO that the ticker count no longer fits in a jbyte), so the new ticker is intentionally not mirrored there. This is safe: toJavaTickerType() -- the only place an unmapped ticker could alias to 0x0 -- has no callers, and Java clients can only reach tickers they can name.

Differential Revision: D114903210

[meta-codesync]

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

[github-actions]

⚠️ clang-tidy: 8 warning(s) on changed lines

Completed in 454.5s.

Summary by check

Check	Count
cert-err58-cpp	8
Total	8

Details

db/internal_stats.cc (8 warning(s))

db/internal_stats.cc:327:26: warning: initialization of 'write_buffer_manager_memory_usage' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:329:26: warning: initialization of 'write_buffer_manager_mutable_memory_usage' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:331:26: warning: initialization of 'write_buffer_manager_buffer_size' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:333:26: warning: initialization of 'write_buffer_manager_stall_active' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:449:35: warning: initialization of 'kWriteBufferManagerMemoryUsage' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:451:35: warning: initialization of 'kWriteBufferManagerMutableMemoryUsage' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:453:35: warning: initialization of 'kWriteBufferManagerBufferSize' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
db/internal_stats.cc:455:35: warning: initialization of 'kWriteBufferManagerStallActive' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 1877265

---

Summary

Well-structured PR adding meaningful observability and a cross-DB flush policy to WriteBufferManager. The design is sound with careful attention to liveness, lock ordering, and shutdown safety. One build-breaking bug (missing ticker in TickersNameMap) and a few style/guideline issues.

High-severity findings (1):

• [monitoring/statistics.cc:346] WRITE_BUFFER_MANAGER_STALL_MICROS is not added to TickersNameMap. The SanityTickers test asserts TickersNameMap.size() == TICKER_ENUM_MAX, so this will fail make check.

Full review (click to expand)

Findings

🔴 HIGH

H1. Missing TickersNameMap entry — monitoring/statistics.cc:346

• Issue: The new WRITE_BUFFER_MANAGER_STALL_MICROS ticker is added to the Tickers enum in include/rocksdb/statistics.h (incrementing TICKER_ENUM_MAX) but is NOT added to the TickersNameMap vector in monitoring/statistics.cc.
• Root cause: The TickersNameMap vector must contain one entry per enum value, in order. The SanityTickers test in monitoring/statistics_test.cc:22 asserts TickersNameMap.size() == TICKER_ENUM_MAX and that each entry's position matches its enum value. Adding a ticker to the enum without a corresponding entry in TickersNameMap will cause this test to fail.
• Suggested fix: Add {WRITE_BUFFER_MANAGER_STALL_MICROS, "rocksdb.write_buffer_manager.stall.micros"}, to TickersNameMap in monitoring/statistics.cc, right after the BLOB_DB_LAZY_PARTIAL_BYTES_SAVED entry (before the closing };). The name rocksdb.write_buffer_manager.stall.micros matches the description in the PR.

🟡 MEDIUM

M1. Defaulted parameter on TEST_AtomicFlushMemTables — db/db_impl/db_impl.h

• Issue: The declaration adds bool non_blocking_write_thread = false as a new defaulted parameter. CLAUDE.md explicitly says: "Avoid new defaulted parameters. This is the Miss Spelling in README #1 trap on refactoring!"
• Suggested fix: Remove the default value and update all call sites to pass the argument explicitly. Alternatively, since this is a TEST_-only method (not production API), the risk is lower, but the guideline applies uniformly.

M2. GetFlushableMemUsage acquires mutex_ on the write path — db/db_impl/db_impl_compaction_flush.cc

• Issue: InitiateFlushOnLargestDB (called from PreprocessWrite, the hot write path) iterates all registered DBs and calls GetFlushableMemUsage() on each, which acquires each DB's mutex_ while holding the WBM registry mutex. With N DBs sharing a WBM, this adds up to N mutex acquisitions per write that crosses the flush threshold.
• Root cause: The cross-DB selection needs current memory data, which is protected by each DB's mutex.
• Suggested fix: Consider caching the flushable memory estimate (e.g., update it on memtable switch rather than re-scanning on every query). The PR description acknowledges this tradeoff, but for large N (dozens of DBs) this could become noticeable. At minimum, document the expected N in the API or the kFlushLargestAcrossDBs doc.

M3. Cross-DB flush path skips MaybeFlushStatsCF — db/db_impl/db_impl_compaction_flush.cc

• Issue: The BackgroundCallWBMFlush path calls FlushMemTableImpl/AtomicFlushMemTablesImpl directly, bypassing HandleWriteBufferManagerFlush which calls MaybeFlushStatsCF. If persist_stats_to_disk is enabled, the stats CF will not be considered for flushing on the cross-DB path.
• Suggested fix: This is likely acceptable since the cross-DB path is specifically for reclaiming memory from the largest CF, not for stats persistence. But worth documenting or at least verifying the stats CF gets flushed through other paths.

🟢 LOW / NIT

L1. CollectFlushableCFs adds !cfd->initialized() check not in old code — db/db_impl/db_impl_compaction_flush.cc

• Issue: The old HandleWriteBufferManagerFlush loop checked only cfd->IsDropped(), while the new CollectFlushableCFs() also checks !cfd->initialized(). This is actually an improvement (matching the SelectColumnFamiliesForAtomicFlush pattern) and not a bug, but worth noting as a semantic change.

L2. FlushableCFs::oldest is tracked but never used on cross-DB path — db/db_impl/db_impl.h

• Issue: CollectFlushableCFs tracks both largest and oldest, but the cross-DB flush path (BackgroundCallWBMFlush) always uses largest. The oldest field is only used in HandleWriteBufferManagerFlush for kFlushOldest policy. Not wrong, but the data member is carried through FlushableCFs even on the cross-DB path where it's unused.

L3. Minor: kWBMFlushPriority is Env::Priority::LOW (compaction pool) — db/db_impl/db_impl.h

• Issue: The comment explains why LOW is chosen over HIGH and BOTTOM, which is well-reasoned. Under sustained heavy compaction load, WBM flush jobs could be delayed. The comments acknowledge this ("The accepted cost is that a job can wait behind queued compactions") which is appropriate.

L4. auto type for lambda — db/db_impl/db_impl_compaction_flush.cc

• Issue: CLAUDE.md says "Avoid declared type auto". The unref_generated_candidates lambda uses const auto. This is the standard C++ pattern for lambdas and is arguably the only reasonable way to declare one, so this is a very minor nit.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (shares PreprocessWrite)	YES (flush policy is orthogonal to txn visibility)	Safe
ReadOnly DB	NO (MaybeRegisterFlushInitiator returns early)	N/A	Safe
User-defined timestamps	YES	YES (flush selection doesn't depend on key format)	Safe
MemPurge	YES	YES (mutable memtable memory is still tracked)	Safe
BlobDB	YES	YES (memtable memory tracked regardless of value location)	Safe
Concurrent writers	YES	YES (write thread join is serialized)	Safe
FIFO / Universal compaction	YES	YES (flush policy is orthogonal to compaction style)	Safe
Atomic flush	YES (special-cased throughout)	YES (total_mem vs largest_mem distinction)	Safe
PauseBackgroundWork	YES (checked in GetFlushableMemUsage and ScheduleWriteBufferManagerFlush)	YES	Safe
WaitForCompact	YES (bg_wbm_flush_scheduled_ added to wait condition)	YES	Safe

Positive Observations

• Liveness argument is carefully constructed: The "self-flush before stall" pattern in PreprocessWrite correctly handles the case where a deferred flush to another DB might not complete, preventing global deadlock.
• Lock ordering is well-documented: The registry mutex -> DB mutex ordering is stated in comments and enforced by the code structure.
• CollectFlushableCFs consolidation: Extracting the CF scanning logic into a single function ensures the bid (GetFlushableMemUsage) and the action (BackgroundCallWBMFlush) agree on what gets flushed.
• Test coverage is excellent: The tests cover key edge cases including stalled DBs, write-stopped DBs, read-only DBs, atomic flush DBs, queued job cancellation on close, and the self-flush-before-stall liveness property.
• Non-blocking write thread join: The EnterUnbatchedNonBlocking implementation correctly handles both the "stall already active" case (via LinkOne's no_slowdown check) and the "stall begins while queued" case (via AwaitState with STATE_COMPLETED mask + BeginWriteStall sweep).
• Shutdown safety: The bg_wbm_flush_scheduled_ counter is properly included in all wait conditions (CloseHelper, WaitForBackgroundWork, PauseBackgroundWork, WaitForCompact).

---

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

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 0e40ee5

---

Summary

Well-structured PR that adds meaningful observability to the WriteBufferManager and introduces a cross-DB flush registry. The non-blocking write thread join design correctly avoids the deadlock scenario. Lock ordering (registry -> DB mutex) is consistently maintained. The flush policy as a WBM-level (not DBOptions) setting is the right design for a shared manager.

High-severity findings (1):

• [db/db_impl/db_impl_compaction_flush.cc: UnscheduleWBMFlushCallback] mutex_.AssertHeld() assertion in the unschedule callback is fragile -- it relies on the fact that UnSchedule is only called from CloseHelper under mutex_, but this invariant is undocumented and the existing UnscheduleFlushCallback does NOT assert this.

Full review (click to expand)

Findings

🔴 HIGH

H1. UnscheduleWBMFlushCallback asserts mutex_ held, but unschedule callbacks are not guaranteed to run under mutex_ -- db_impl_compaction_flush.cc

• Issue: UnscheduleWBMFlushCallback asserts fta->db_->mutex_.AssertHeld(). The thread pool's UnSchedule() calls unschedule callbacks outside its own lock (threadpool_imp.cc:457-459). The assertion holds today only because CloseHelper calls env_->UnSchedule(...) while holding mutex_ (lines 846-850). However, the existing UnscheduleFlushCallback for regular flushes does NOT assert mutex_ held -- it directly decrements the counter. If any future code path calls UnSchedule for this tag without holding mutex_, debug builds will crash.
• Root cause: Implicit coupling between the unschedule callback and the caller's lock state, diverging from the established pattern.
• Suggested fix: Either (a) remove the mutex_.AssertHeld() and make bg_wbm_flush_scheduled_-- + bg_cv_.SignalAll() safe without mutex_ (matching UnscheduleFlushCallback's pattern), or (b) add a clear comment in CloseHelper documenting that UnSchedule for WBM tasks MUST be called under mutex_.

🟡 MEDIUM

M1. TOCTOU gap in cross-DB flush selection -- db_impl_compaction_flush.cc:GetFlushableMemUsage + ScheduleWriteBufferManagerFlush

• Issue: InitiateFlushOnLargestDB calls GetFlushableMemUsage() (which checks write_controller_.IsStopped(), WouldBlockJoiningWriteThread(), etc.) and then calls ScheduleFlush(). Between these two calls, the winning DB's state can change (e.g., writes get stopped, a stall begins). ScheduleWriteBufferManagerFlush rechecks most conditions and BackgroundCallWBMFlush rechecks again. The three-layer rechecking is thorough, but the bid can be wasted, causing the caller to unnecessarily defer.
• Root cause: Inherent race in cross-process coordination with lock-free state.
• Suggested fix: This is defense-in-depth and acceptable for correctness. The fallback path (caller flushes itself if deferred flush fails) handles the gap. Consider adding a comment noting this intentional TOCTOU.

M2. CollectFlushableCFs skips IsFlushPendingOrRunning CFs from largest/oldest but still counts them in total_mem -- db_impl_compaction_flush.cc

• Issue: CFs with IsFlushPendingOrRunning() are skipped for largest and oldest selection but their mem is still added to total_mem. For atomic flush bidding (which uses total_mem), this means the bid includes memory from CFs that won't actually be switched, potentially inflating the bid and winning selection when the DB can't actually reclaim that much memory.
• Root cause: total_mem counts all non-empty mutable memtable memory, but atomic flush's SelectColumnFamiliesForAtomicFlush may skip some CFs.
• Suggested fix: Either (a) clarify in a comment that total_mem is an upper bound and that's acceptable for bid ranking, or (b) track separately the total memory of CFs that would actually be switched.

M3. FlushInitiator is in a public header but is internal -- include/rocksdb/write_buffer_manager.h

• Issue: FlushInitiator is documented as "Internal adapter for selecting and flushing a DB sharing a WBM." StallInterface already follows this same pattern, but FlushInitiator's virtual methods (GetFlushableMemUsage, ScheduleFlush) expose internal implementation details. Users subclassing it would create maintenance obligations.
• Root cause: Following the existing StallInterface pattern.
• Suggested fix: Add a comment like StallInterface has: "intended for RocksDB internal use only."

M4. No test for two_write_queues_ with non-blocking join -- db_write_test.cc

• Issue: EnterUnbatchedNonBlocking has a code path for two_write_queues_ in FlushMemTableImpl and AtomicFlushMemTablesImpl where it joins both write_thread_ and nonmem_write_thread_. If the first succeeds but the second fails, it exits the first. This reversal path is not tested.
• Root cause: The test EnterUnbatchedNonBlockingRefusesDuringStall only tests a single write thread.
• Suggested fix: Add a test that stalls only the nonmem write thread and verifies that a non-blocking join correctly exits the primary write thread when the secondary join fails.

M5. Release note only mentions WriteBufferFlushPolicy -- unreleased_history/public_api_changes/write_buffer_manager_flush_policy.md

• Issue: The release note mentions the flush policy and its three modes but doesn't mention the four new DB properties (kWriteBufferManager*) or the new WRITE_BUFFER_MANAGER_STALL_MICROS ticker. These are user-facing API additions that users would want to know about.
• Suggested fix: Add a separate release note under new_features/ for the properties and the stall ticker, keeping it concise per CLAUDE.md guidance.

🟢 LOW / NIT

L1. flush_policy_ uses memory_order_relaxed -- write_buffer_manager.h

• Issue: SetFlushPolicy and flush_policy() both use memory_order_relaxed. A stale read means one flush uses the old policy. This is acceptable since the policy is not latched to any other state.
• Suggested fix: Document the intentional relaxed ordering in a comment.

L2. total_mem in CollectFlushableCFs for atomic flush -- naming clarity

• Issue: total_mem in FlushableCFs represents "total mutable memtable memory across all non-dropped, initialized, non-empty CFs" but its field name doesn't convey that it includes CFs already flushing. For non-atomic flush total_mem is unused (only largest_mem matters).
• Suggested fix: Add a brief comment on the total_mem field.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
ReadOnly DB	YES (GetFlushableMemUsage returns 0)	YES	Safe -- MaybeRegisterFlushInitiator correctly skips
WritePreparedTxnDB	YES (via PreprocessWrite)	YES	No visibility interaction
Atomic flush DB	YES	YES	BackgroundCallWBMFlush handles both paths
Pipelined write	YES	YES	EnterUnbatchedNonBlocking calls WaitForMemTableWriters
two_write_queues_	YES	YES (but reversal path untested)	See M4
CompactedDBImpl	NO (read-only, never hits PreprocessWrite)	N/A	Safe
Shutdown during WBM flush	YES	YES	bg_wbm_flush_scheduled_ properly drained
CF dropped during WBM flush	YES	YES	BackgroundCallWBMFlush Refs the CF

Positive Observations

• The three-layer eligibility checking (GetFlushableMemUsage -> ScheduleWriteBufferManagerFlush -> BackgroundCallWBMFlush) provides thorough defense-in-depth against races.
• The unref_generated_candidates lambda in AtomicFlushMemTablesImpl correctly reduces code duplication from the original three separate unref blocks.
• Lock ordering (flush_initiators_mu_ -> mutex_) is documented and consistently followed throughout.
• Test coverage is comprehensive with good use of sync points for deterministic concurrency testing.
• The FlushLargestAcrossDBsSelfFlushesBeforeStall test specifically validates the deadlock-prevention claim.
• The EnterUnbatchedNonBlockingSweptOutByLaterStall test validates the tricky swept-writer edge case.
• Using LOW priority for WBM flushes correctly avoids competing with the flush pool (HIGH), meaning WBM flushes proceed even when the HIGH pool is saturated.
• The Java ticker exhaustion issue is handled correctly: toJavaTickerType has no callers, so the unmapped ticker is safe.

---

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