feat(mem_wal): drive the WAL append on a flush-interval ticker#7894
Merged
hamersaw merged 4 commits intoJul 22, 2026
Conversation
`flush_interval_ms` was inert. It routed to a timer that was only ever evaluated *on the write path*, so it could add a redundant trigger but never delay or batch one — and since every durable put triggered its own append, tuning the knob did nothing at all. Give the WAL flusher a real ticker (`MessageHandler::tickers`, which the memtable flusher already used) and take the append off the put path. The append is the only thing on that schedule: it is an S3 PUT, billed per call, and bounding that cost is the entire reason a flush interval exists. The index apply is not on it — it is in-memory and free to batch, so it stays per-put on its own task. The tick names no store. `MessageFactory` is synchronous and cannot take the async state lock, and capturing an `Arc<BatchStore>` at handler construction would pin the first memtable forever. So `WalFlushSource::NextPending` is resolved when the message is *handled* — by walking the live stores oldest-first and taking the first that still owes an append. Oldest-first, not "the active memtable", and this is load-bearing. WAL entry positions are assigned in append-call order; replay walks them ascending; row positions follow; and primary-key recency is "newest visible row position wins". So append order *is* dedup order. A tick enqueued before a freeze is handled after it, and resolving to the active memtable would append the incoming memtable's batches ahead of the outgoing one's tail — silently handing the key to the stale row on the next replay. It survives the crash that caused it, and a full scan cannot see it. Selecting by cursor makes the target a function of what is durable rather than of when the timer fired. Two starvation fixes in the dispatcher, both of which this makes reachable: - The interval used the default `MissedTickBehavior::Burst`, which replays every tick missed while `handle()` ran. A WAL append easily outlasts its own interval, so missed ticks accumulate, the ticker arm is always ready, and — being `biased` — it starves the channel. Now `Delay`. - The `biased` select polled the ticker *before* `rx.recv()`. A tick only ever adds an append a real trigger would have made anyway, whereas a message may be a freeze's completion cell or `close()`'s final append, which nothing else delivers. Messages now win. `durable_write: true` with no ticker is rejected at open: the put path no longer triggers its own append, so such a writer would block forever on an append that never comes. Better a clear error than a deadlock. Accepted cost: single-client sequential *durable* throughput drops from ~10 writes/sec (one PUT round-trip) to roughly one per tick. That is the policy choice — the interval should mean what it says, and API cost should be bounded. Latency-sensitive callers want `durable_write: false`, which now costs them durability only, not visibility. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`sync_indexed_write`, `async_index_buffer_rows`, and `async_index_interval` had no behavioral consumers — they were only serialized into the config-defaults metadata map. They existed to configure async index-update buffering, which the index-apply-task split replaced with unconditional per-put indexing: a write is now read-your-writes through the index in every mode, which is exactly what `sync_indexed_write` promised. The knobs no longer control anything. Remove all three across every surface: the Rust core field, builders, defaults, and metadata serialization; the write-throughput bench's now-meaningless sync/async index axis; and the Python and Java binding parameters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
A-python
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMemWAL removes synchronous and asynchronous indexed-write configuration across Rust, Java, and Python APIs. WAL appends now use ordered background flushing with ticker-based scheduling, and benchmarks and tests are updated for durability-based behavior. ChangesMemWAL core and validation
Language bindings
Benchmarks
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShardWriter
participant TaskDispatcher
participant WalFlushHandler
participant WalFlusher
ShardWriter->>TaskDispatcher: defer WAL append or flush
TaskDispatcher->>WalFlushHandler: deliver ticker or explicit message
WalFlushHandler->>WalFlusher: flush oldest pending BatchStore
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/python/tests/test_mem_wal.py (1)
210-214: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
sync_indexed_writethrough a deprecation period.
Dataset.mem_wal_writerno longer accepts this public keyword, so existing callers fail with an unexpected-keywordTypeError. Keep it as a deprecated compatibility argument, warn that it has no effect, and add warning coverage before its eventual removal.
python/python/tests/test_mem_wal.py#L210-L214: retain the legacy-keyword invocation and assert its deprecation behavior.python/python/tests/test_mem_wal.py#L355-L360: retain compatibility coverage for the durable e2e writer setup.python/python/tests/test_mem_wal.py#L407-L407: retain compatibility coverage for reopening a writer with the legacy keyword.As per coding guidelines, “Do not break public API signatures; deprecate old APIs with
#[deprecated]or@deprecatedand add a replacement.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/tests/test_mem_wal.py` around lines 210 - 214, Restore sync_indexed_write as a deprecated, accepted keyword in Dataset.mem_wal_writer, emit a deprecation warning stating that it has no effect, and preserve the writer behavior otherwise. In python/python/tests/test_mem_wal.py at lines 210-214, assert the warning while retaining the legacy invocation; retain compatibility coverage at lines 355-360 and 407-407 for durable setup and reopening with the keyword.Source: Coding guidelines
rust/lance/src/dataset/mem_wal/write.rs (1)
1493-1643: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject durable writes without a flush ticker
durable_write = truewithmax_wal_flush_interval = Nonecan leave puts waiting forever when the buffer never hits the size trigger. Add this check inopen()beforeclaim_epoch().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/mem_wal/write.rs` around lines 1493 - 1643, The ShardWriter::open flow must reject durable writes when no periodic flush is configured, since size-triggered flushing alone can leave puts waiting indefinitely. Before manifest_store.claim_epoch(), validate that config.durable_write requires config.max_wal_flush_interval to be Some and return an invalid-input error otherwise; leave non-durable configurations and durable configurations with an interval unchanged.
🟡 Other comments (1)
rust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rs-635-643 (1)
635-643: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a zero flush interval for durable read mode.
shard_writer_configconvertsmax_wal_flush_interval_ms=0to no ticker, then Line 643 forcesdurable_write=true; the writer is rejected. Validate this combination before construction and includemax_wal_flush_interval_ms=0in the error.As per coding guidelines, “Validate inputs at API boundaries and reject invalid values with descriptive errors; never silently clamp or adjust them.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rs` around lines 635 - 643, Validate the durable-read configuration before constructing the writer in the benchmark setup around shard_writer_config: when durable_write is forced true and max_wal_flush_interval_ms is 0, reject the input with a descriptive error explicitly naming max_wal_flush_interval_ms=0. Keep valid configurations flowing through shard_writer_config unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/python/tests/test_mem_wal.py`:
- Around line 210-214: Restore sync_indexed_write as a deprecated, accepted
keyword in Dataset.mem_wal_writer, emit a deprecation warning stating that it
has no effect, and preserve the writer behavior otherwise. In
python/python/tests/test_mem_wal.py at lines 210-214, assert the warning while
retaining the legacy invocation; retain compatibility coverage at lines 355-360
and 407-407 for durable setup and reopening with the keyword.
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 1493-1643: The ShardWriter::open flow must reject durable writes
when no periodic flush is configured, since size-triggered flushing alone can
leave puts waiting indefinitely. Before manifest_store.claim_epoch(), validate
that config.durable_write requires config.max_wal_flush_interval to be Some and
return an invalid-input error otherwise; leave non-durable configurations and
durable configurations with an interval unchanged.
---
Other comments:
In `@rust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rs`:
- Around line 635-643: Validate the durable-read configuration before
constructing the writer in the benchmark setup around shard_writer_config: when
durable_write is forced true and max_wal_flush_interval_ms is 0, reject the
input with a descriptive error explicitly naming max_wal_flush_interval_ms=0.
Keep valid configurations flowing through shard_writer_config unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 62b04814-4680-4ff4-9cba-83eed110271f
📒 Files selected for processing (19)
java/lance-jni/src/mem_wal.rsjava/src/main/java/org/lance/memwal/ShardWriterConfig.javajava/src/test/java/org/lance/memwal/MemWalTest.javapython/python/lance/dataset.pypython/python/tests/test_mem_wal.pypython/src/dataset.rsrust/lance/benches/mem_wal/fts/mem_wal_fineweb_fts.rsrust/lance/benches/mem_wal/fts/mem_wal_fts_read_bench.rsrust/lance/benches/mem_wal/kv/mem_wal_kv_point_lookup.rsrust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rsrust/lance/benches/mem_wal/vector/hnsw/mem_wal_recall_hnsw.rsrust/lance/benches/mem_wal/vector/mem_wal_index_micro.rsrust/lance/benches/mem_wal/vector/mem_wal_vector_bench.rsrust/lance/benches/mem_wal/write/mem_wal_replay.rsrust/lance/benches/mem_wal/write/mem_wal_shard_writer_backpressure.rsrust/lance/benches/mem_wal/write/mem_wal_write.rsrust/lance/src/dataset/mem_wal/api.rsrust/lance/src/dataset/mem_wal/wal.rsrust/lance/src/dataset/mem_wal/write.rs
💤 Files with no reviewable changes (14)
- java/src/test/java/org/lance/memwal/MemWalTest.java
- rust/lance/benches/mem_wal/vector/mem_wal_index_micro.rs
- rust/lance/benches/mem_wal/vector/hnsw/mem_wal_recall_hnsw.rs
- rust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rs
- java/lance-jni/src/mem_wal.rs
- rust/lance/benches/mem_wal/fts/mem_wal_fts_read_bench.rs
- rust/lance/benches/mem_wal/kv/mem_wal_kv_point_lookup.rs
- rust/lance/benches/mem_wal/write/mem_wal_replay.rs
- rust/lance/benches/mem_wal/vector/mem_wal_vector_bench.rs
- rust/lance/src/dataset/mem_wal/api.rs
- java/src/main/java/org/lance/memwal/ShardWriterConfig.java
- rust/lance/benches/mem_wal/write/mem_wal_shard_writer_backpressure.rs
- python/python/lance/dataset.py
- python/src/dataset.rs
`test_open_rejects_durable_write_without_a_ticker` asserted that `ShardWriter::open` rejects `durable_write: true` with no `max_wal_flush_interval`, but the validation was never added — the test failed on every platform (windows-build and linux-arm in CI), Linux just reported later. Since memtable puts no longer self-trigger a WAL append, a durable put becomes visible only once the background ticker drives the append. With no interval (or a zero one, which tokio can't schedule) a small put that never fills the size-triggered buffer blocks until close. Validate this at open, before claiming the epoch, matching the neighboring index/enable_memtable check. WAL-only mode is exempt: there a durable put flushes inline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/dataset/mem_wal/write.rs (1)
6021-6039: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover the zero-duration rejection and assert the error type.
This test covers
None, but the new validation also rejectsSome(Duration::ZERO). Add that case and assert the invalid-input variant as well as the message so either branch cannot regress silently.As per coding guidelines, “Every bugfix and feature must have corresponding tests” and “Assert on both the error variant and the message content in tests; do not check only
is_err().”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/mem_wal/write.rs` around lines 6021 - 6039, The test test_open_rejects_durable_write_without_a_ticker currently covers only max_wal_flush_interval: None. Extend it to also exercise Some(Duration::ZERO), and for both invalid configurations assert the specific invalid-input error variant plus a message containing max_wal_flush_interval, preserving the existing rejection assertion.Source: Coding guidelines
🟡 Other comments (1)
rust/lance/src/dataset/mem_wal/write.rs-1508-1524 (1)
1508-1524: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new valid-value contract on
max_wal_flush_interval.The public config docs do not state that durable MemTable mode requires
Some(duration > Duration::ZERO), nor link this constraint to [ShardWriter::open]. Callers can otherwise configure a value that now fails only at open time.As per coding guidelines, “Add doc comments to public API elements that convey semantic meaning, valid values, and effects; do not restate type signatures.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/mem_wal/write.rs` around lines 1508 - 1524, Update the public documentation for max_wal_flush_interval to state that durable MemTable mode requires Some(duration greater than Duration::ZERO), and link this validation behavior to ShardWriter::open. Describe the semantic constraint and its effect without restating the field’s type signature.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 6021-6039: The test
test_open_rejects_durable_write_without_a_ticker currently covers only
max_wal_flush_interval: None. Extend it to also exercise Some(Duration::ZERO),
and for both invalid configurations assert the specific invalid-input error
variant plus a message containing max_wal_flush_interval, preserving the
existing rejection assertion.
---
Other comments:
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 1508-1524: Update the public documentation for
max_wal_flush_interval to state that durable MemTable mode requires
Some(duration greater than Duration::ZERO), and link this validation behavior to
ShardWriter::open. Describe the semantic constraint and its effect without
restating the field’s type signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 3832fbfe-9f8e-4d77-a747-ed92f0b20afd
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/write.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
WAL-only mode still flushed the WAL inline on every durable put — one S3 PUT per put — while MemTable mode had already moved durable appends onto the background ticker. That asymmetry was an incomplete migration, not a design choice: at lance-format#6675 both modes flushed per durable put, and the ticker commit only migrated MemTable mode, leaving WAL-only on a per-trigger done cell. The done cell existed solely to propagate flush errors back to the caller; since the two-cursor work, BatchDurableWatcher::wait() surfaces the typed fence/persistence error via the poison latch, so the done cell is no longer needed. Make WAL-only consistent: - open_wal_only_mode wires max_wal_flush_interval into the flush handler and shares the pending WalOnlyState so a tick resolves against it. - flush_from_wal_only advances the writer-global durability cursor after a successful append (the FIFO queue's front sits at the watermark, so the new watermark is durable + count). - put_wal_only drops the inline done-cell flush and instead parks durable puts on the durability watermark, triggering the size/time flush for all puts like MemTable mode. A terminal append failure poisons the writer and wakes every waiter with the typed error. - The open() rejection of durable_write with no ticker now applies to both modes (the WAL-only carve-out is gone). Concurrent durable puts on a fenced writer now all wake via the poison latch rather than each racing its own flush. Adds a focused test that a durable WAL-only put is durable the instant put() returns, and parametrizes the no-ticker rejection over both modes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WAL flush-interval ticker
The feature the original #7791 was titled for, now small because it sits on the read-visibility redesign (#7888) that landed ahead of it.
flush_interval_mswas inert: it routed to a timer only ever evaluated on the write path, so it could add a redundant trigger but never delay or batch one.MessageHandler::tickers) and take the append off the put path. The append is the only thing on that schedule — it's an S3 PUT, billed per call, and bounding that cost is the whole reason a flush interval exists. The index apply stays per-put on its own task.WalFlushSource::NextPendingis resolved when the message is handled, by walking live stores oldest-first and taking the first that still owes an append.sync_indexed_writeconfig family —sync_indexed_write,async_index_buffer_rows, andasync_index_intervalhad no behavioral consumers after the index-apply-task split (fix(mem_wal): read-your-writes via split index-apply and dual visibility cursors #7888) made every write read-your-writes. Removed across Rust core, benches, and Python/Java bindings.Stack — this is the last of three; the first two have merged into
main:mainCloses the original monolithic #7791.
🤖 Generated with Claude Code