raft: cut per-write durability barriers 12.38 -> 4.50 (two gated fixes) - #51
Merged
Conversation
…FAULT The write-path flatten stack and the raft barrier-coalescing fixes have been gated default-OFF since they landed. They are now live-proven on a 3-node cluster, so make them the default and keep the env var only as an escape hatch. Adds an `env_flag_default_on` / `wal_env_flag_default_on` / `raft_env_flag_default_on` sibling to each module: the gate reads as ON unless explicitly disabled with `=0|false|no|off`. Now default-ON: TS_PHASE1_FLAT (engine.rs, wal.rs) TS_ENGINE_CONCURRENT_COMMIT (engine.rs) TS_RAFT_APPLY_COALESCE (engine.rs) TS_RAFT_WAL_COALESCE (raft.rs) TS_RAFT_PROPOSE_SERIALIZE (raft.rs) Already default-ON and unchanged: TS_GROUP_COMMIT, and the single WAL durability barrier (wal_single_barrier() = !wal_legacy_recovery()). TS_INDEX_CATALOG_FOLD is deliberately NOT flipped: its own contract is "ships dark; flips on after the crash-recovery suite is green", and that precondition is not met yet. Tests: a gate-OFF assertion can no longer express "off" by leaving the variable unset, so the three baselines that did now disable the gate explicitly (concurrent_commit, phase1_flat, raft_apply_coalesce). Adds EnvFlagGuard::off() for the same reason in the raft suite. Measured on a 3-node c7i.xlarge cluster with these defaults and no env vars set: shared path 1.017 fdatasync/write at C=1 and 0.175 at C=16 (group commit coalescing ~6 writes per barrier); raft 14.74 fdatasync/write vs 58.84 with the gates forced off, write p50 79.5ms vs 202.2ms, write QPS 12.5 vs 5.0, leader read p50 0.75ms vs 13.34ms.
A deployed raft process owns ONE node but keeps a full cluster view, so `wal_records()` emitted a record per peer and `persist_configured_wal()` took a barrier for every one of them. A single propose also calls that function several times as the log appends and then the commit index advances. Measured on a 3-node cluster: 12.38 fdatasync per write, and 0.00 per read. P3 -- TS_RAFT_PERSIST_LOCAL_ONLY, default OFF Adds `local_node_id` to the cluster inner state, set by the deployed runtime (one node per process) and left unset by the in-process test cluster, which genuinely hosts every node. When set, the persist covers only this node's own record. A peer makes its own hard-state durable before answering an RPC and re-learns the rest from AppendEntries, so persisting it here buys no safety. P4 -- TS_RAFT_PERSIST_DEFER, default OFF Nested persists record that a barrier is owed; one flush covers the final state before the propose acks. Intermediate states are subsumed by the final one, and a crash before the ack drops an entry that was never acked. Enabled only while the propose serialize lock is held, so exactly one writer owns the deferral, and the flush error is propagated on the success path: a write whose barrier failed is never acked. Measured on 3x c7i.xlarge, one binary, gates enabled separately: baseline 12.38 fsync/write p50 67.0ms 14.9 QPS C=32 3575ms 6.7 QPS +P3 7.88 fsync/write p50 48.9ms 20.5 QPS C=32 2340ms 11.2 QPS +P3+P4 4.50 fsync/write p50 37.6ms 26.6 QPS C=32 1620ms 17.1 QPS 2.75x fewer barriers, 1.8x write latency, 2.55x throughput at C=32. Latency is barrier-bound here, so cutting barriers moves latency and scale together. P4 lives on the distributed propose path, which the in-process test cluster does not exercise, so it is verified on a real cluster rather than by a unit test. P3 is unit-tested: only the local node's record grows while peers stay flat, and the local node still takes its own barrier. Also adds the env lock this test file has always needed -- about ten of its tests mutate process-global gates with no serialisation, which made them fail unpredictably under the parallel runner.
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.
Cuts raft's per-write durability barriers from 12.38 to 4.50, measured on a
3-node cluster.
Where the barriers came from. A deployed raft process owns exactly one node
but keeps a full cluster view, so
wal_records()emitted a record per peer andpersist_configured_wal()took a barrier for every one of them. A singlepropose also calls that function several times — once as the log appends, again
as the commit index advances. Three nodes x ~4 calls = ~12.
P3 —
TS_RAFT_PERSIST_LOCAL_ONLY(default OFF)Adds
local_node_idto the cluster inner state, set by the deployed runtime(one node per process) and left unset by the in-process test cluster, which
genuinely hosts every node and must still persist them all. When set, the
persist covers only this node's own record.
Safe because a peer makes its own hard-state durable before answering an RPC and
re-learns the rest from AppendEntries on restart — the leader persisting it
buys no Raft safety.
P4 —
TS_RAFT_PERSIST_DEFER(default OFF)Nested persists record that a barrier is owed; one flush covers the final
state before the propose acks. Intermediate states are subsumed by the final
one, and a crash before the ack drops an entry that was never acked.
Two details that matter for correctness:
exactly one writer owns it — otherwise concurrent proposes would interleave
the flag
(
(Ok(_), Err(err)) => Err(err)), so a write whose barrier failed is neveracked. A
Drop-based flush would have swallowed that error, which is why thepropose function is split rather than wrapped in a guard
Measured — 3x c7i.xlarge, one binary, gates enabled separately so each is
attributed on its own:
2.75x fewer barriers, 1.8x write latency, 2.55x throughput at C=32. Latency is
barrier-bound here, so cutting barriers moves latency and scale together.
Reads take zero barriers in every config (measured with a separate read-only
window).
Testing. P4 lives on the distributed propose path, which the in-process test
cluster does not exercise (
propose()goes viapropose_one), so it isverified on a real cluster rather than by a unit test — worth knowing before
relying on CI alone for it. P3 is unit-tested: only the local node's record
grows while peers stay flat, and the local node still takes its own barrier.
174 raft tests pass with both gates on and with both off.
Also adds the env lock
raft/tests/part4.rshas always needed — about ten ofits tests mutate process-global gates with no serialisation, which made them
fail unpredictably under the parallel runner.
Still on the table: 4.50 is above the ~2 design target. P4 only defers
inside
propose_distributed_one; the apply path and tick handlers still taketheir own barriers. Two dead ends worth recording so they are not retried —
TS_RAFT_PROPOSE_SERIALIZEis not the bottleneck (on vs off is identical:75.78 vs 75.83 ms, 12.38 fsync both), and heartbeat tuning does nothing
(100/20/5 ms all land at ~83 ms).