Repository navigation
keeper slow member backpressure 4lw - #117728
Conversation
A write commits once a quorum acknowledges it, so a replica slower than the quorum is never waited for: the leader keeps running ahead until that replica is so far behind that it can only be repaired by a full snapshot. `bpon` asks the leader to stop advancing the commit index past any replica it can reach, so that a replica which fell behind can close the gap instead of drifting. `bpof` turns that off. Writes then commit at the speed of the slowest reachable replica, so this is meant to be switched on deliberately and switched off again once the replicas have caught up. Nothing is automatic: no lag threshold, no trend detection, no time limit. Every replica the leader can reach is waited for, however far behind it is. A replica is left out only when the leader cannot reach it at all - its last request failed, it has not answered within four heartbeat intervals, or it has not answered at all yet on a new connection - because waiting cannot help such a replica and would freeze the commit index for nothing. A replica receiving a snapshot is waited for. The transfer is no faster for waiting, but the log stops growing, so the replica does not have to catch up on everything written during the transfer and then need another snapshot. One new setting, `slow_member_backpressure_max_uncommitted_log_entries`: the value `nuraft_max_uncommitted_log_entries` takes while the backpressure is on. Holding the commit index back does not stop the leader appending, so without a tighter limit the log runs further ahead and the replica falls further behind instead of catching up. `0` leaves the limit unchanged, and if both limits are `0` a warning is logged at startup, because then nothing bounds the log while the commit index is held. Either command can be sent to any node: a follower forwards it to the current leader, which applies it and propagates it to its peers on a best-effort basis. Only the leader's setting has any effect, but every node reports its own in `mntr` as `zk_slow_member_backpressure`, which is what an operator checks. The setting lasts for one leadership - a new leader switches it off - and is not persisted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er that makes no progress Only the leader acts on the switch, so only the leader reports it in `mntr`. A `bpon` sent to a follower is forwarded to the leader and does not switch anything locally, and a server that loses leadership switches it off. A follower reporting it on would tell an operator the cluster is throttled when it may not be. Add `slow_member_backpressure_no_progress_timeout_ms`: a peer that makes no progress at all within the timeout is no longer waited for. It defaults to five minutes, which is long enough for a Keeper snapshot transfer, where the peer's matched index stays frozen until the transfer finishes. The integration test asserts the clamp the leader logs rather than requiring every throttled write to succeed. While the backpressure is on, writes past `slow_member_backpressure_max_uncommitted_log_entries` are refused by design, so clients are expected to see timeouts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
Workflow [PR], commit [5d78884] Summary: ❌
AI ReviewSummaryAdds Final Verdict
LLVM Coverage ReportMeasured on commit 5d78884.
Changed lines: Changed C/C++ lines covered: 70/71 (98.59%) · Uncovered code |
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Object file sizes34 object files changed (+53.71 KiB total), 0 added.
716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only Compile time of recompiled translation units64 translation units recompiled, 340 s compile time in total, 64 of them have a recent master baseline. |
…ldLeadershipCommand` Adding `SlowMemberBackpressureOnCommand` above `YieldLeadershipCommand` left the comment describing the latter in front of the former, so one command was documented twice over and the other not at all.
While the slow member backpressure is on, the leader left out any replica whose matched log index is zero, on the grounds that it does not know where such a replica stands and waiting for it could freeze the commit index for nothing. Zero is also what a reconnection leaves behind, though: NuRaft resets the matched index when it remakes the connection to a replica, and writes it again only once a snapshot install completes. A replica that restarts, reconnects and then needs a snapshot therefore reported nothing for the whole transfer and was left out, so the leader kept committing on the majority and could outrun it again - the drift that `bpon` exists to stop, and the opposite of what the documentation promised for a replica receiving a snapshot. The other path into a snapshot, a replica that stays connected and falls behind the start of the log, kept its matched index and was waited for correctly. A replica that has been down long enough to need a snapshot has lost its connection by definition, so the broken path is the one operators meet. A snapshot on its way to a replica is what tells the leader that zero is not ignorance: it knows exactly what the replica holds, which is nothing of the log yet. There is no index to clamp to, so the leader holds where it already is. A replica still probing after a reconnection has no snapshot on the way and is left out as before. Reported by a review of #117728
`requestSlowMemberBackpressure` returns `true` for two different outcomes. On the leader it applied the setting, and the answer is a fact. On a follower it did no more than hand a custom notification to the node it currently believes leads, and the leader refuses one that arrives after it has stopped leading, so the setting may never come on at all. The reply told an operator the same thing either way: that writes now commit at the speed of the slowest reachable replica. On a follower that is a claim about the cluster the node is in no position to make, and it is the claim an operator acts on - the whole reason to type `bpon` is to accept the slowdown deliberately. Read the setting back before saying anything about writes. Only the leader holds it, so it can read back on only where it was actually applied, and a follower gets an answer that says what it did and where to confirm it. This is not the same as asking whether this node leads, which can change between the two calls. Reported by a review of #117728
When the leader can send a replica neither the log entries it needs nor a snapshot to replace them with, it warns the replica that it is out of range and sends nothing else. Replication cannot recover such a replica at all, so `bpon` waiting for it only stalls writes. None of the reasons to leave a replica out caught the case. The warning is a request like any other and the replica answers it, and an accepted answer is what resets the response timer, so the replica kept looking like it was making progress; its matched index is whatever it reached before falling out of range, so it looked known; and its requests did not fail, so it looked reachable. The freeze then sustained itself. Snapshots are created from the committed index, so a leader that cannot commit never reaches the next one, and without a new snapshot the replica never leaves the range it is stuck outside of. Nothing in the ordinary run of the cluster breaks the loop - it takes `bpof`, a forced snapshot, or a custom commit quorum, all of which need someone to notice first. Reported by a review of #117728
`MonitorCommand::print` writes `zk_` before every key, so `slow_member_backpressure` and `server_state` are names that never appear in the output. The `bpon` reply sent an operator looking for both of them, and the documentation repeated the wording. The same page was already inconsistent with itself: its `mntr` sample prints `zk_server_state` and `zk_slow_member_backpressure`, and the upgrade instructions further down use `zk_server_state`. Reported by a review of #117728
Both places on the page that give the default value of `four_letter_word_white_list` were out of date. They were already missing `rclc`, `clrs`, `ftfl`, `pfev` and `lgrq`, the jemalloc commands were never mentioned, and `bpon` and `bpof` have now been added to `DEFAULT_FOUR_LETTER_WORD_CMD` as well - so the page told readers to allow commands that answer without being allowed. Also say that the list is read at startup, because an operator who changes it and reloads the configuration will find that nothing happened. Reported by a review of #117728
`apply_slow_member_backpressure` iterates only regular members, so a learner is never held, and `learner_is_never_held_test` pins that. A Keeper replica configured with `can_become_leader` set to `false` is exactly that: the configuration is passed to NuRaft as the learner flag. The reply and the documentation said writes commit at the speed of the slowest reachable replica, without saying which replicas that covers. An operator running read replicas that way would have read it as a promise that the backpressure protects them, when they can still drift until they need a snapshot - and a lagging read replica is the case that most invites reaching for `bpon`. Say which replicas are waited for instead of widening the implementation. Excluding a learner is deliberate: the commit index is decided by the replicas that vote, and holding it for one that does not vote is a different bargain from the one the command offers - a permanently slow read replica would stall the cluster, which is usually why it is not a voter. Reported by a review of #117728
`can_become_leader` `false` and `priority` `0` are easy to confuse and only one of them costs a replica the backpressure. A zero-priority replica does not initiate an election but still votes, so the leader waits for it; a replica with `can_become_leader` `false` is a learner with no vote and is never waited for.
antonio2368
left a comment
There was a problem hiding this comment.
Don't forget to update to master commit before merging
Resolve the `contrib/NuRaft` conflict by pointing at NuRaft `master` (`c9c180b`), which is the merge of the `backpressure-all-or-nothing` pull request the branch used to reference directly. It contains both sides: the branch's `12b4945` and master's `a0eda00`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@groeneai review keeper stress tests run results |
Keeper stress results for this PR
Final head against those 87 heads (
All 15 runs sit inside the control p10-p90 band, and the spread across this PR's own heads (write-multi 8065 to 8740 rps) is wider than the distance to the control median, so the NuRaft bump shows no throughput or latency cost. Post-merge master (merge
One limit worth naming: the stress suite never switches the feature on. For completeness, the reds on the final head are not Keeper: the amd_msan AST fuzzer abort is the master-wide |
ClickHouse/NuRaft#126
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Add slow member backpressure 4LW in clickhouse-keeper.
Workflow [PR]
Sync PR [sync-upstream/pr/117728]