perf(mem_wal)!: serve the shard manifest from the store that wrote it - #8640
Open
hamersaw wants to merge 7 commits into
Open
perf(mem_wal)!: serve the shard manifest from the store that wrote it#8640hamersaw wants to merge 7 commits into
hamersaw wants to merge 7 commits into
Conversation
`WriteStats` already tracks flush counts and cumulative time, but a running total cannot be resampled into a distribution — the individual observations are gone by the time anything polls it. An embedder can compute an average and nothing else, which is exactly the wrong shape for latency: a flush pipeline is judged on its tail, not its mean. Observe each flush individually instead, through the `metrics` facade that `lance-io` already uses for object store operations. Observations route to whatever `Recorder` the embedding process installed, so this crate takes no position on the exporter and the emit sites compile away with the feature off. One family with a `kind` label rather than two: a WAL buffer flush and a memtable flush are stages of the same write pipeline and are read together, even though they differ by orders of magnitude — hence bucket bounds spanning a single object-store round trip through a multi-second dataset write. Counts and byte totals stay on `WriteStats`. They are cumulative and lose nothing to sampling, so there is no reason to route them through a recorder.
Replaces the `metrics`-facade approach from the previous commit. The problem is unchanged: `WriteStats` tracks flush counts and cumulative time, but a running total cannot be resampled into a distribution. An embedder can compute an average and nothing else, which is the wrong shape for latency — a flush pipeline is judged on its tail, not its mean. Report each flush to an optional `WalObserver` on `ShardWriterConfig`, alongside `warmer`. The consumer supplies the sink and owns the aggregation, so Lance still takes no position on the exporter, but now needs no feature flag and no process-global recorder. An injected sink rather than the facade because the consumer holds context Lance does not — the table a shard belongs to, in particular, which a process-global histogram cannot label. It also matches how consumers already reach into this config: `SsTableWarmer` and `DatasetCache` cross the same boundary the same way, while the facade has one producer in the tree and no consumer that installs a recorder. Every trait method defaults to a no-op, so adding an event later is not a breaking change for implementors. Counts and byte totals stay on `WriteStats`: they are cumulative and lose nothing to sampling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The write bench builds `ShardWriterConfig` field-by-field with no `..default()`, so adding `observer` broke every job that checks benchmarks: clippy, MSRV, build-no-lock, and the "Check benchmarks" step on mac and windows. Set it to `None` beside the sibling `warmer`. Add the test the observer commit was missing. A durable put returns only once its WAL flush landed, and the seal fence resolves only once the sealed memtable reached L0, so both callbacks have fired by the time it asserts — no sleeping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ShardManifestStore::read_latest` scanned the version space on every call: a GET for `version_hint.json`, a HEAD to confirm the hinted version, then batches of parallel HEADs until a whole batch 404s. The loop's exit condition guarantees that terminating batch is all misses, so the floor is `2 + manifest_scan_batch_size` requests even when the hint is exact — paid per read, because nothing cached the result. Cache the manifest a store durably wrote and serve it from `read_latest`. Soundness does not rest on holding the claim: manifest versions are CAS-allocated and gap-free, since every writer commits `latest.version + 1` with PUT-IF-NOT-EXISTS. A successful write at version N therefore proves N was the tip — a peer cannot hold N+1 without N existing first. So any store may serve what it wrote, which also lets the replay tailer reuse the cursor it just stamped. Only a write populates the cache. A read miss deliberately does not, so a reader-only handle still observes the writer; caching reads instead made a `WalTailer` pin the first manifest it saw and never see the cursor advance. A failed CAS invalidates, which both frees `commit_update`'s retry to re-read storage and keeps a possibly-fenced writer from trusting itself. `check_fenced` and `claim_epoch` read through `read_latest_uncached`: both exist to observe another process, which our own cache can never show us. Also expose `ShardWriter::manifest_store` so an embedder commits through the same instance the writer uses — two stores over one shard would keep two caches, and neither would see the other's commits. Measured against a WAL node, per read of a fresh tier with N generations: `5 + N` object-store requests before, `N` after. Replay of 200 unflushed WAL entries: 8.81 requests per entry before, 4.18 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flush task and the WAL tailer's cursor updates share one `ShardManifestStore`, so two writes can win their CAS in one order and return to their callers in the other: the loser re-reads storage, commits the next version and caches it, then the winner's slow response finally seats the older one. Nothing is overwritten — versions are CAS-allocated and gap-free, so a stale cache can only ever propose a version that is already taken — but `ShardWriter::manifest()` would under-report a just-flushed generation until the next commit. Cache only a higher version. Also state the staleness contract on `ShardWriter::manifest()`, and cut the comments the manifest cache added roughly in half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hamersaw
marked this pull request as ready for review
August 20, 2026 15:05
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
hamersaw
force-pushed
the
refactor/mem-wal-manifest-cache
branch
from
August 20, 2026 20:17
1005733 to
e18676c
Compare
hamersaw
force-pushed
the
refactor/mem-wal-manifest-cache
branch
from
August 20, 2026 20:51
e18676c to
2d15779
Compare
wjones127
self-requested a review
August 20, 2026 22:04
hamersaw
force-pushed
the
refactor/mem-wal-manifest-cache
branch
from
August 21, 2026 11:41
2d15779 to
93bb4b5
Compare
The manifest cache serves a landed write as proof of the tip, which holds only while versions are gap-free. Nothing enforced that: every caller hand-wrote `current.version + 1` and nothing checked the result, so a cached commit could be acknowledged behind the durable tip. The gap is load-bearing beyond the cache. `find_latest_version` stops at the first absent batch, so a gap wider than `manifest_scan_batch_size` with a lost best-effort hint makes even an uncached read misreport the tip. A store can only check contiguity against a position it holds, and the tailer was the one writer that could never hold one — it writes no epoch, and its store must serve fresh reads for its own position hints. So it goes first: - `WalTailer` tracks the highest position it has read in memory, which is all `next_position()` ever needed. `best_effort_cursor_update` is gone. - Publishing `wal_entry_position_last_seen` moves to the replay driver, which already holds the epoch, as an ordinary `commit_update`. That drops one manifest write per replayed WAL entry, and leaves every manifest writer an epoch holder. That makes the store's own position a sound baseline for serving: - `latest()` serves the position when held, else scans — and a scan here is deliberately not adopted, so a reader that polls keeps observing the writer instead of pinning the first manifest it saw. - `refresh_latest()` scans and adopts what it finds. A claim reads uncached precisely because it must see another process, and the tip it finds is what its own write then builds on. The successor check belongs with whoever holds the predecessor, not with the store's position: that position is shared, and a peer's failed CAS clears it, so a commit could be rejected as a gap over an empty position midway through. `commit_update` checks the closure's output against the manifest the closure received, which is immutable and local. `write` keeps only what its own state can judge — a version at or below its position is reported as the collision it is, so callers retry. BREAKING CHANGE: `ShardManifestStore::read_latest` is renamed to `latest`, `read_latest_uncached` to `refresh_latest`, and `write` is now crate-private — callers reach it through `commit_update`, `claim_epoch`, or `initialize_shard`, which derive versions from a manifest they just read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hamersaw
force-pushed
the
refactor/mem-wal-manifest-cache
branch
from
August 21, 2026 12:45
93bb4b5 to
49fd79c
Compare
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The manifest mutation boundary now validates each update against the exact predecessor supplied to its closure, while collision-driven cache invalidation remains only a retry concern. The raw-gap and stale-predecessor races are both closed without restoring storage reads to the hot path.
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
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.
Problem
ShardManifestStore::read_latestscanned the version space on every call: a GET forversion_hint.json, a HEAD to confirm the hinted version, then batches of parallel HEADs until a whole batch 404s. The loop only exits on a full batch of misses, so the floor is2 + manifest_scan_batch_sizeobject-store requests even when the hint is exact — and nothing cached the result, so a reader paid it per request.Profiling a WAL node, this was essentially all of its GET+HEAD traffic. Per fresh-tier read with N L0 generations:
5 + Nrequests, of which the 5 were manifest probing and only N were data.Change
Serve the manifest from the store that wrote it, and make the invariant that permits this an enforced one rather than an assumed one.
Serving
A store keeps the manifest it durably wrote as its position and serves that from
latest(). Soundness does not rest on holding the claim. Manifest versions are CAS-allocated and gap-free — every writer commitslatest.version + 1under PUT-IF-NOT-EXISTS — so a successful write at version N proves N was the tip: a peer cannot hold N+1 without N existing first.The two reads now differ by whether they take a position, which is what the names say:
latest()refresh_latest()A plain scan deliberately does not adopt, so a reader that polls keeps observing the writer rather than pinning the first manifest it saw.
refresh_latestadopts because a claim reads uncached precisely to see another process, and the tip it finds is what its own write then builds on.check_fencedandclaim_epochuse it for exactly that reason.Enforcing
Nothing previously enforced the gap-free invariant the cache rests on: every caller hand-wrote
current.version + 1and nothing checked the result. Given a gap, a cached commit could be acknowledged behind the durable tip.The invariant is also load-bearing well beyond the cache.
find_latest_versionstops at the first absent batch, so a gap wider thanmanifest_scan_batch_sizecombined with a lost best-effort hint makes even an uncached read misreport the tip.The check belongs with whoever holds the predecessor, not with the store's position — that position is shared, and a peer's failed CAS clears it, so a commit can find itself judged against an empty position midway through.
commit_updatetherefore validates the closure's output against the manifest the closure received, which is immutable and local. A version the caller did not intend is rejected, not silently corrected;ShardManifest::next_version()is what callers build with.writekeeps only what its own state can judge: a version at or below this store's position is reported as the collision it is, so callers retry.The tailer
A store can only check contiguity against a position it holds, and
WalTailerwas the one writer that could never hold one: it claims no epoch, and its store must serve fresh reads for its own position hints — so no single piece of state could be both stable enough to validate against and fresh enough to hint from.It turns out it never needed to write at all. Its per-entry manifest write maintained
wal_entry_position_last_seenpurely as a cursor hint fornext_position(), whose only callers are tests; replay, the tailer's sole production user, derives the tip from its own read loop. The tailer now tracks the highest position it has read in memory, and publishing that cursor moves to the replay driver, which already holds the epoch, as an ordinarycommit_update.That removes one manifest write per replayed WAL entry and leaves every manifest writer an epoch holder.
Measured
Against a WAL node driving this code:
5 + NrequestsNThe replay figure predates the tailer change, which removes a further manifest write per entry.
Writes are unchanged (one conditional PUT per entry).
Breaking changes
ShardManifestStore::read_latest→latestShardManifestStore::read_latest_uncached→refresh_latestShardManifestStore::writeis now crate-private. Callers reach it throughcommit_update,claim_epoch, orinitialize_shard— the three entrances that derive a version from a manifest they just read. It was public from the commit that introduced MemWAL and never acquired a caller.Downstream
commit_updateclosures need no change: settingversion: current.version + 1is exactly what the check expects.Tests
cargo test -p lance --lib dataset::mem_wal::— 619 passed.Covering the properties this is allowed to break:
check_fencedthrough a held positionlatest()keeps observing the writer, and never adopts a positionrefresh_latestadopts — reversing that would either pin pollers or reject valid claimscommit_updaterecovers from a stale position instead of spinning on the version it lostNote on the commit stack
The first two commits are pre-existing work from the flush-observer branch that has not landed upstream yet; read this PR as its final commit. Rebasing once those merge will reduce it to the single change.
🤖 Generated with Claude Code