feat(partitions): persist segment log and sparse index concurrently - #3970
feat(partitions): persist segment log and sparse index concurrently#3970hubcio wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3970 +/- ##
=============================================
- Coverage 84.75% 67.59% -17.16%
Complexity 1405 1405
=============================================
Files 1224 1223 -1
Lines 176743 151415 -25328
Branches 143057 117732 -25325
=============================================
- Hits 149804 102354 -47450
- Misses 22956 45137 +22181
+ Partials 3983 3924 -59
🚀 New features to boost your workflow:
|
6fb6cbb to
2d6081e
Compare
hubcio
changed the title
2d6081e to
bd76371
Compare
Every journal flush issued two serialized fdatasyncs, log then index, so an fsync-gated topic paid two device round trips per ack. Both files now persist under one futures::future::join: 13.4k -> 26.1k msg/s and p50 5.83 -> 3.01 ms on ext4 with enforce_fsync and messages_required_to_save = 1, 8 producers into one partition. The cursors advance only after both saves succeed, so a failed half keeps its slot and the retry overwrites it; MessagesWriter::rewind lost its last caller and is gone. Recovery had to change first. The old order was a barrier only under enforce_fsync = true: with the shipped default the page cache already wrote the two inodes back in any order, and recovery's own pass C truncates the log before the index, so an index holding entries the log does not back was already ordinary. Boot refused it as IndexLogDivergence, and on a single-replica node that refusal was a permanent tombstone for a partition whose log was healthy. An index entry is derived from the log, so it is never evidence the log lacks. Recovery now steps back to the highest entry the log proves and floors the index there; when no entry proves, or the entries contradict each other (out of order, or below the segment start), it drops the index and rebuilds from the log. Nothing a client was promised is lost, since no ack precedes persist, and the walk still runs its residue probe, so a survivor past damage refuses as before. That walk now checksums every batch it accepts, because unordered files mean a durable index entry no longer implies a durable chunk and one chunk can hold several batches. The step-back is async, yields on refills and charges the shared probe budget, so a crafted tail refuses as UnverifiedResidue instead of stalling boot; FileScanner::slice_at anchors its backward moves to keep refills linear. An integration test covers the shape: a three-node cluster, one node SIGKILLed after an eager flush, its log cut behind the last index entry, then restarted. It asserts the floor marker in that node's stdout, the exact floored index length while the log is still exactly the cut length, the surviving prefix served and the node caught up. It is red before the recovery change and green with it. Measuring this needed three benchmark fixes. Reports took min, max and std_dev from the per-bucket moving average series, which averages the extremes away while the percentiles beside them used the raw samples; all three now come from those same sorted samples. The balanced-producer kind refused fewer than two partitions, but one is a legal target and the shape a durability benchmark needs. And --enforce-fsync claimed to fsync every write while the topic's flush threshold (server default 1024) still governed when the journal reached disk, so acks were not durability-gated unless it was also 1; that threshold is now exposed as --messages-required-to-save, plumbed into topic creation, named in the fsync help and shown in an example.
A sealed segment clones a cached descriptor on a poll, but the active segment had no read-state slot and reopened its log file on every single poll. Give it a slot too, filled on the first read miss and dropped when the segment seals. The active descriptor stays outside the sealed LRU. That cache is keyed by start offset and budgeted at twelve entries, so admitting the active segment would let unrelated sealed traffic evict it. Its lifetime is bound instead to the two sealed-ness transitions, rotation and the state-transfer un-seal. Only the descriptor is cached. The sparse index and the offset memo stay sealed-only because they assume an immutable file, and the active segment grows under the reader. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The legacy-report backfill tested min, max and standard deviation independently, so a run whose samples are all equal had its genuine zero standard deviation refilled from the per-bucket moving average. That put back the averaged-away extremes the raw-sample summary exists to avoid. Reports written before the three fields existed receive them from serde's default together, so all three reading zero is what identifies such a report. Key the backfill on that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither save_indexes nor save_frozen_batches has a caller outside the crate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The anchor search keyed its yield on the scan window refilling. An index entry whose position is past the log end is rejected on arithmetic alone and reads nothing from it, so it never triggers a refill, and an index floored back to a short log is mostly those entries. The search therefore held the shard core, signal handling included, for one pread per entry across the whole file. Yield every ANCHOR_SEARCH_YIELD_STRIDE entries as well, and lift the refill check out of the inner loop so it is reached on every entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bd76371 to
a1dac8b
Compare
hubcio
changed the title
| // Refusing it handed a benign torn tail a permanent tombstone | ||
| // on a single-replica node. Step down | ||
| // to the highest entry the log still proves and floor the | ||
| // index there, so pass C drops the entries nothing backs. | ||
| let anchor = find_provable_index_anchor( |
There was a problem hiding this comment.
The step-back can anchor above interior damage, and nothing ever walks [0, anchor.position). refuse_if_survivor_past_damage only scans forward from walk.position and returns Ok immediately when damage_position >= messages_size (:1757), so damage below the anchor is invisible to it.
Reachable without hand-crafting an index. One entry per flushed chunk is what the writer emits; at-rest bit rot inside chunk 3, plus the same crash tearing the tail so the last entry cannot prove. The step-back lands on chunk 4, which verifies, and the walk runs from there to EOF and returns Ok. The rot at chunk 3 is never read. Result: a checksum-failing batch inside the advertised offset range, stats.increment_messages_count at :339 counting offsets that cannot be served, and current_offset written into the advance-only superblock. master refused these bytes.
Suggest the floored path fall through to recover_by_walking_log (walk from byte 0) instead of a second anchored walk. That reaches the probe with damage_position < messages_size, finds the survivor, and refuses with InteriorDamage with the bytes preserved.
Related: the justification at :1080-1085 ("an entry the log cannot back is a torn tail of the INDEX, never proof that the log lost data") is false in exactly this case, since the unprovable tail entry is evidence the log lost an interior page.
| found_offset: header.base_offset, | ||
| position, | ||
| }), | ||
| let mut walk = walk_chain_from_anchor( |
There was a problem hiding this comment.
The walk still starts at last.position, so [0, last.position) is accepted on index evidence alone, which is the evidence this PR itself declares insufficient (server_error.rs:288-294, and :1370-1377 here).
On master that was safe for a specific reason: save_frozen_batches fsyncs the whole file (messages_writer.rs:129-131), not a range, so under the old serial order "index entry N is durable" implied "log fdatasync N completed" implied "log bytes [0, N) are all durable". The futures::future::join in iggy_partition.rs deletes that implication. So under enforce_fsync = true this is newly unsound rather than pre-existing, and the premise now applies to every entry rather than only the last.
stats.increment_messages_count(end_offset - start_offset + 1) at :339 then reports a count over a span nothing read.
Suggest checksum-walking from byte 0 whenever the index was floored or the segment was not cleanly sealed. At minimum, stop deriving messages_count from an unwalked span.
| // failure: it advances its write cursors only once both the batch | ||
| // and the index are durable, so the retry overwrites the same | ||
| // positions instead of appending a duplicate. Chunks already durable |
There was a problem hiding this comment.
There is no retry. The only production consumer of a persist Err is commit_messages_inner -> commit_partition_entry (sets failed_commit, returns false) -> handle_committed_entries, which panics at :3246. Nothing re-invokes the persist against the un-advanced cursors. The shutdown path (flush_committed_messages) warns and moves to the next namespace, and state_transfer re-arms the transfer rather than the persist.
That panic also does not do what its own comment promises. The shard message pump runs inside compio::runtime::spawn (bootstrap.rs:1274), which this repo documents as silently swallowing spawned-task panics (core/message_bus/tests/installer_panic_cleanup.rs:20-27, and the two production scopeguards built around it at installer/replica.rs:424-430 and installer/tcp.rs:112-117). There is no panic = "abort" and no custom hook. So an ENOSPC or EIO unwinds one shard's pump and the process stays up: the listener keeps accepting, other shards keep running, and every partition on that shard silently stops committing, ticking and replying. ShardPumpDied is only raised by drain_shard_pump, which runs at shutdown.
The cursor discipline itself is worth keeping: it is what makes the shutdown-flush warn-and-continue path non-corrupting, and boot recovery is the real retry since SegmentStorage::new reopens at the recovered lengths. Suggest dropping the retry framing here and at :3743-3749 and documenting boot recovery as the sole recovery, then fixing the wedge separately.
3 participants
Concurrent log and index fsync
Every journal flush issued two serialized fdatasyncs, log then index, so an fsync-gated topic paid two device round trips per ack. Both files now persist under one
futures::future::join: 13.4k to 26.1k msg/s and p50 5.83 to 3.01 ms on ext4 withenforce_fsyncandmessages_required_to_save = 1, 8 producers into one partition. Cursors advance only after both saves succeed, so a failed half keeps its slot and the retry overwrites it.Recovery had to change first. The old ordering was a barrier only under
enforce_fsync = true; with the shipped default the page cache already wrote the two inodes back in any order, and recovery's own pass C truncates the log before the index, so an index holding entries the log does not back was already ordinary. Boot refused it asIndexLogDivergence, which on a single-replica node permanently tombstoned a partition whose log was healthy. An index entry is derived from the log, so it is never evidence the log lacks: recovery now steps back to the highest entry the log proves and floors the index there, or rebuilds from the log when no entry proves or the entries contradict each other. No ack precedes persist, so nothing promised is lost, and the walk still runs its residue probe. That walk now checksums every batch it accepts, because unordered files mean a durable index entry no longer implies a durable chunk. The step-back is async and charges the shared probe budget, so a crafted tail refuses asUnverifiedResidueinstead of stalling boot.Wire format change: misrouted partition writes are forwarded
Please read this section before approving.
The roster advertises the metadata plane's primary only. Both planes derive their primary from the view number modulo the replica count, with no per-plane offset, and their views advance independently. Once they diverge, every partition write routed by the roster reaches a node that is not that group's primary, and the client's fallback rechecks the same roster. That closed loop is why the symptom was a livelock rather than an error, and why a three-node cluster could not accept writes in that state.
A backup now forwards the client's request frame to the partition primary. The forward direction needed no new type, since a routed request already crosses the peer wire. The return direction did: a reply is refused as client-bound on the peer path, and the client routing key encodes an owning shard rather than a node, so a remote primary cannot answer the producer. The origin keeps the connection, parks the request by nonce, and the primary answers with a new
ForwardRequestResultframe at discriminant 30. The forward is fire and forget, because the rejection site runs inside the shard pump where a cross-node round trip would stall every partition on that shard.This makes the release non-rolling, extending the declaration the existing forwarding commands already carry; a mixed fleet drops the new frame and the origin waits out its timeout. The at-least-once window also widens, since the partition plane has no per-client dedup, so a forward that times out and is replayed by the SDK commits again at a new offset. Writes only: the result envelope is sized for a write reply.
Three findings on this commit are known and unaddressed. Five rejection paths on the primary answer a client they do not hold, so the origin pays its full timeout before the producer sees a transient error (a stall, not a loss). A forwarded write skips client-ingress authorization, consistent with the trusted-peer model but currently implicit. And if the origin has no encryptor while the primary does, the payload is written in plaintext.
Also in this PR
The active segment now fills the same per-segment read-state slot that sealed segments use. It was populated for sealed segments only, so a consumer following the head paid one
openatper poll, each an io_uring operation prone to an io-wq punt. It is demand-filled on the first miss, dropped when the segment seals, and deliberately outside the sealed LRU budget so unrelated traffic cannot evict it. Only the descriptor is cached, never the sparse index, because the active segment grows under the reader.Three smaller fixes: latency extremes are backfilled only when all three of min, max and standard deviation are absent, since backfilling individually let a report mix a raw value with two derived ones. The segment writer save methods are narrowed to the crate, where their only callers are, so nothing opens a second write path past the cursor discipline above. And the index anchor search yields while walking a floored index, which recovery can now leave shorter than the log expects.
Tests and verification
Two integration tests. One SIGKILLs a node after an eager flush with its log cut behind the last index entry, then asserts the floor marker, the exact floored index length against the unchanged log length, the surviving prefix served, and the node caught up. The other kills the view-0 primary of both planes before any partition group exists, waits out an election, restarts it, then creates a topic whose group starts at view 0 while the metadata view has moved, asserting that both the dialed node and the advertised leader accept the write. Each send is budgeted so the livelock reports as a failure instead of hanging.
fmt, sort and clippy clean with all features and targets. partitions 127 tests, bench-report 22, server 311. The routing test passes against a server binary rebuilt from the branch head, which matters because the integration crate does not rebuild it. The fd cache commit builds standalone in a detached worktree, so the split bisects.