feat(mem_wal): expose frozen memtable and backpressure stats - #8241
Conversation
The MemWAL writer keeps rich in-process counters, but two things an operator needs during an incident are unreachable from outside it: * How much memory the writer owes to flush. `MemTableStats` reports the active memtable only, so the backpressure threshold (`max_unflushed_memtable_bytes`, which meters active + frozen) has no observable numerator. Adds `frozen_count` and `frozen_bytes`, read under the read lock `memtable_stats` already holds. Deliberately not `in_memory_memtable_refs`, which calls `check_poisoned` first and so goes blank exactly when the numbers matter. The two fields have different denominators on purpose: bytes drain the moment a flush commits (that is what backpressure meters), while the handle lingers in the read view for `frozen_memtable_grace`. * Whether writes are being throttled at all. `BackpressureStats::stats` and `snapshot` are already public, but the controller sits inside a private `WriterMode` variant with no accessor. Adds `ShardWriter::backpressure_stats`, answering in both writer modes so a caller never has to know which one it is in. Both are additive and read-only. Sophon's WAL service polls them to publish `wal_unflushed_bytes`, `wal_sealed_memtables`, `wal_backpressure_waits_total` and `wal_backpressure_wait_seconds_total`, none of which it can derive today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b8f412b to
2deb7df
Compare
`closed_memtable_stats` builds a `MemTableStats` by hand, so adding `frozen_count`/`frozen_bytes` to the struct broke the pylance build. The python crate is outside the workspace, so `cargo check --workspace` did not catch it. Both fields are zero for a closed writer: close awaits every frozen memtable's flush, so nothing is owed. Zero them on the early-return path too, where the active memtable had no buffered batches but frozen ones may still have been flushed by close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new surface needs to preserve truthful incident-state telemetry: uncommitted frozen memory must remain represented after a failed flush, and an ongoing first backpressure wait must be observable before it completes. A viable revision can separate authoritative live gauges from cumulative completed-wait counters instead of exposing bookkeeping whose failure semantics differ from the advertised metrics.
Please mark this PR with the breaking-change label.
| /// `frozen_count` it excludes in-grace tables. Plus the active memtable's | ||
| /// `estimated_size`, this is what backpressure meters against | ||
| /// `max_unflushed_memtable_bytes`. | ||
| pub frozen_bytes: usize, |
There was a problem hiding this comment.
frozen_bytes becomes zero after a failed flush even though the same frozen table remains resident and uncommitted. The flush handler unconditionally subtracts its size, then only removes or stamps the retained entry on success. This contradicts “heap bytes still owed to flush” and makes poison-time wal_unflushed_bytes under-report precisely when this snapshot is intended to remain useful.
Keep an authoritative uncommitted/resident byte gauge separate from the backpressure-drain bookkeeping (for example, sum retained entries whose flushed_at_ms is None under the existing read lock), and cover the failed-flush state.
Reproducer
I temporarily extended the existing deterministic test_frozen_retained_after_failed_flush after its failed drain:
let failed_stats = writer_a.memtable_stats().await.unwrap();
assert_eq!(failed_stats.frozen_count, 1);
assert_eq!(failed_stats.frozen_bytes, 0);cargo test -p lance --lib test_frozen_retained_after_failed_flush passed. The retained table still held rows, but the snapshot observed count 1 and bytes 0; the advertised unflushed-memory contract requires those retained bytes to remain represented.
There was a problem hiding this comment.
Fixed in 30102ef: frozen_bytes now sums retained tables whose flushed_at_ms is None, so failed or otherwise uncommitted tables remain represented while successfully flushed grace entries are excluded. The targeted failed-flush test passes on this head. This finding is fixed.
|
|
||
| /// Snapshot of the backpressure counters. Both writer modes answer, so a | ||
| /// caller asking "am I throttled" need not know which mode it is in. | ||
| pub fn backpressure_stats(&self) -> BackpressureStatsSnapshot { |
There was a problem hiding this comment.
This accessor reports only completed waits: BackpressureStats::record runs only when the loop later observes a value below the threshold. During a first in-progress wait, the snapshot remains 0/0, so it cannot answer the documented “am I throttled” question or expose the stated live-throttling incident.
Track active waiter state or current elapsed wait separately (with cancellation-safe accounting), while keeping completed-wait totals explicitly distinct.
Reproducer
I temporarily ran this focused async test against the observed head:
let controller = Arc::new(BackpressureController::new(config));
let worker = Arc::clone(&controller);
let task = tokio::spawn(async move {
worker.maybe_apply_backpressure(|| (1000, None)).await.unwrap();
});
tokio::time::sleep(Duration::from_millis(30)).await;
let snapshot = controller.stats().snapshot();
assert_eq!(snapshot.total_count, 0);
assert_eq!(snapshot.total_wait_ms, 0);
task.abort();cargo test -p lance --lib gate_verify_in_progress_backpressure_is_not_counted passed, confirming that the actively throttled call was invisible to the exported counters.
There was a problem hiding this comment.
Fixed in 30102ef: a drop guard now tracks active_count for the full wait and releases it on normal completion or cancellation, so a first in-progress wait is observable without conflating it with completed totals. The focused backpressure tests pass on this head. This finding is fixed.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review found both new signals go blank in the incident they exist for. `frozen_bytes` read `frozen_memtable_bytes`, the backpressure counter, which drains whenever a flush *completes* — success or failure. A failed flush leaves the memtable resident and un-stamped in the read view but reported zero bytes owed. Sum the retained un-stamped entries instead; identical in every other case, since both sides are `estimated_size()`. The counter's unconditional drain stays as is: its watcher is popped in the same block, so bytes that stuck around would leave the next `put` spinning on the no-watcher sleep branch forever. A hang is worse than a leak. That leaves resident memory from failed flushes unmetered by backpressure, which is a write-path change and not this PR's business — `frozen_bytes` now makes the gap visible, and the field documents it. `BackpressureStats::record` only fires when the wait loop exits under threshold, so a writer parked in its first stall reported 0/0 — the one question the accessor was added to answer. Add `active_count`, held by a drop guard so a caller cancelled mid-wait cannot strand a phantom waiter, and say plainly that the totals are completed waits only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Gate recommendation: approve.
2 fixed / 0 remain. The revised implementation preserves failed-flush memory in the observable backlog and exposes active backpressure with cancellation-safe accounting.
Please mark this PR with the breaking-change label.
What
Two additive, read-only accessors on the MemWAL writer.
MemTableStats.frozen_count/.frozen_bytes—memtable_statsreports the active memtable only, so the backpressure threshold (max_unflushed_memtable_bytes, which meters active + frozen) has no observable numerator. Both are read under the read lockmemtable_statsalready holds.Deliberately not
in_memory_memtable_refs: that callscheck_poisoned()?first, so it goes blank exactly when an operator most needs the numbers.memtable_statsis already documented as poison-tolerant for that reason.The two fields have different denominators on purpose, and the doc comments say so:
frozen_bytesdrains the moment a flush commits (that is what backpressure meters), while the handle lingers in the read view forfrozen_memtable_grace, sofrozen_countdoes not drop to zero with it.ShardWriter::backpressure_stats()—BackpressureController::stats()andBackpressureStats::snapshot()are already public, but the controller sits inside a privateWriterModevariant with no accessor, so the counters are unreachable from outside the writer. Answers in both modes, so a caller never has to know which one it is in.Why
LanceDB's WAL service polls these to publish four Prometheus metrics it cannot derive today:
wal_unflushed_bytes,wal_sealed_memtables,wal_backpressure_waits_total,wal_backpressure_wait_seconds_total. Without them a WAL pod can be throttling, or sitting a hair under an OOM, with nothing on a dashboard to say so.Tests
test_memtable_stats_frozen_count_outlives_frozen_bytes— pins the differing-denominator semantics: afterwait_for_flush_drainunder a long grace, count is non-zero and bytes are zero.test_backpressure_stats_reachable_in_both_modes—rstestoverenable_memtabletrue/false, since the point of the accessor is that the private-variant match covers both.cargo test -p lance --lib -- mem_wal→ 561 passed, 1 ignored.cargo fmt --allclean. Clippy is clean on the changed file;-D warningscurrently fails elsewhere in the tree on pre-existingsingle_range_in_vec_initunder Rust 1.97.Scope
rust/lance/src/dataset/mem_wal/write.rsonly. No format change, nothing inlance-core, no public signature altered — only new fields on a struct and one new method.🤖 Generated with Claude Code