feat(mem_wal): flush active memtable on ShardWriter::close#6717
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Pin moves from lance-format/lance#6715 merge SHA to lance-format/lance#6717's rev, so `close_merge_insert_writers()` now flushes the active memtable into a Lance fragment instead of dropping it. Required for sustained-throughput benchmarks that intentionally raise `max_memtable_size` above the workload size to measure pure put rate vs close-time drain separately.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
hamersaw
left a comment
hamersaw
left a comment
There was a problem hiding this comment.
Thanks for the PR! This looks great, ease of life things.
| // Errors here are logged inside freeze_memtable and | ||
| // its message-send is fire-and-forget; we still | ||
| // try to drain anything already frozen. | ||
| let _ = writer_state.freeze_memtable(&mut st); | ||
| } |
There was a problem hiding this comment.
The comment says that freeze_memtable logs errors internally, but I don't think that is accurate. So we are essentially dropping the error here without any visability. I think it makes sense to either (1) make sure we actually log at the "error" level or (2) capture and throw the error. API-wise I think it would make sense to have error thrown if I call shard_writer.close() and it is not successful.
There was a problem hiding this comment.
Good catch — fixed in the latest push. freeze_memtable does not log internally, so the comment was wrong and silently dropping the error was wrong. Reworked the code to propagate the error: freeze_memtable(&mut st)? from inside close(). Since close() already returns Result<()>, the freeze failure now surfaces to the caller, as you suggested.
| // close() must flush the memtable into a Lance generation. | ||
| writer.close().await.unwrap(); | ||
| } |
There was a problem hiding this comment.
I think this just validates that close completes. Should we a few steps further and validate that (1) close executions without errors (if we're throwing from close call), (2) the currently active memtable now has 0 rows, and (3) the L0 cached memtable (on-disk) exists?
There was a problem hiding this comment.
Beefed up the test. It now:
- asserts
close()returnsOkexplicitly (expect("close() must succeed and propagate any freeze/flush error")), - reopens the same shard via a second
ShardWriter::open, - reads the persisted manifest and asserts
manifest.current_generation > initial_gen— direct evidence that close() drove a MemTable flush + manifest commit, not just dropped the rows on the floor.
I considered also asserting the reopened active memtable's row_count == 0, but that wedges into WAL-replay behavior (replay still rehydrates entries because state.last_flushed_wal_entry_position isn't updated as WAL flushes happen — it's only initialized from the manifest at open). That looks like a separate replay-coordination issue and test_memtable_replay_* is the right home for it. Happy to file a follow-up if you want it tracked.
Previously, `ShardWriter::close()` only drained the WAL — the active memtable was dropped along with the writer without producing a Lance fragment. Data was durable in the WAL (recoverable on replay), but no LSM-level generation was created. That's surprising for callers who reasonably expect close() to make all data fully durable in the LSM sense, and it makes flush-cost benchmarks impossible: a workload that doesn't cross `max_memtable_size` during puts has nothing to measure at close. Change `close()` to freeze the active memtable (if non-empty) and await every frozen memtable's flush completion before shutting down the task handlers. WAL drain still runs first, so WAL ordering is unchanged. Adds a regression test (`test_close_flushes_active_memtable`) that drives 50 puts with `max_memtable_size = usize::MAX`, asserts no auto-flush happened during puts, and verifies close completes without hanging — without this change close would return promptly but no Lance generation would exist; with it, close awaits the actual fragment write.
f0eca19 to
3d9b429
Compare
|
@hamersaw @jackye1995 updated the PR based on comments if you can take a look |
|
I will post another PR for all the hnsw improvements I am doing based on testing in the past few days |
Switch from the close-flush rev pin to the tagged v7.0.0-beta.8 release. The release includes lance-format/lance#6717 (close flushes the active memtable) and #6715 (flush-task-tree defensive fixes), both required by the LSM merge_insert dispatch.
3 participants
Summary
ShardWriter::close()previously drained the WAL but left the active memtable in memory at shutdown. Any rows that hadn't crossedmax_memtable_size/is_batch_store_fulltriggers were durable in the WAL (recoverable on replay) but never produced a Lance fragment — so no LSM-level generation existed for them.That's surprising semantics for callers who reasonably expect
close()to make all data fully durable in the LSM sense, and it makes flush-cost benchmarks impossible: a workload that intentionally sets a hugemax_memtable_sizeso puts measure pure steady-state throughput has no flush to time at close.Change
Make
close()freeze the active memtable (if non-empty) and await every frozen memtable's flush completion before shutting the task handlers down. WAL drain still runs first, so WAL ordering is unchanged.Test
test_close_flushes_active_memtabledrives 50 puts withmax_memtable_size = usize::MAX, asserts no auto-flush fired during puts (precondition), then verifiesclose()completes without hanging. Without this change close returns promptly but no Lance generation exists; with it, close awaits the actual fragment write.Full
cargo test -p lance --lib mem_wal::is 241 passed, 1 ignored.