Skip to content

keeper slow member backpressure 4lw - #117728

Merged
kssenii merged 11 commits into
masterfrom
keeper-backpressure-all-or-nothing
Sep 11, 2026
Merged

kssenii merged 11 commits into
masterfrom
keeper-backpressure-all-or-nothing

Conversation

@kssenii

@kssenii kssenii commented Sep 2, 2026 •

Copy link
Copy Markdown
Member

ClickHouse/NuRaft#126

Changelog category (leave one):

  • New Feature

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]

kssenii and others added 2 commits September 2, 2026 12:32
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>
@mintlify

mintlify Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
ClickHouse-docs 🟢 Ready View Preview Sep 10, 2026, 12:33 PM

@clickhouse-gh

clickhouse-gh Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [5d78884]

Summary: ❌

job_name test_name status info comment
AST fuzzer (amd_msan) FAIL
Logical error: Can't determine table for parallel replicas (STID: 3634-41e5) FAIL cidb, issue ISSUE CREATED
Integration tests (amd_tsan, 2/6) ERROR
test_storage_nats/test_nats_jet_stream.py::test_nats_jet_stream_hands_back_the_backlog_of_a_consumer_a_direct_select_left_subscribed FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_jet_stream_resumes_consuming_after_two_broker_restarts FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_jet_stream_does_not_resubscribe_while_healthy FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_jet_stream_settles_after_one_broker_restart FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_jet_stream_resumes_consuming_multiple_subjects_after_broker_restart FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_no_connection_at_startup_1 FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_no_connection_at_startup_2 FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_format_factory_settings FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_bad_args FAIL cidb IGNORED
test_storage_nats/test_nats_jet_stream.py::test_nats_drop_mv FAIL cidb IGNORED
2 more test cases not shown

AI Review

Summary

Adds bpon/bpof slow-member backpressure commands for ClickHouse Keeper, the associated coordination settings and mntr field, documentation, and targeted tests. I rechecked the current head against the resolved review threads and did not find remaining correctness, safety, or contract issues in the implementation as it stands.

Final Verdict
  • Status: ✅ Approve

LLVM Coverage Report

Measured on commit 5d78884.

Metric Baseline Current Δ
Lines 88.90% 88.90% +0.00%
Functions 91.80% 91.80% +0.00%
Branches 81.30% 81.30% +0.00%

Changed lines: Changed C/C++ lines covered: 70/71 (98.59%) · Uncovered code

Full report · Diff report

@clickhouse-gh clickhouse-gh Bot added pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR. labels Sep 2, 2026
@clickhouse-gh

clickhouse-gh Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing 5d78884e3 with master cf7992904 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Object file sizes

34 object files changed (+53.71 KiB total), 0 added.

Object file Master PR Δ
contrib/nuraft-cmake/CMakeFiles/_nuraft.dir/__/NuRaft/src/raft_server.cxx.o 330.12 KiB 347.70 KiB +17.57 KiB (+5.32%)

716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only clickhouse-bundle) and not compared.

Compile time of recompiled translation units

64 translation units recompiled, 340 s compile time in total, 64 of them have a recent master baseline.

Job report

…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
Comment thread src/Coordination/FourLetterCommand.cpp Resolved
`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
Comment thread contrib/NuRaft Resolved
Comment thread src/Coordination/FourLetterCommand.cpp Resolved
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
Comment thread src/Coordination/FourLetterCommand.cpp Resolved
`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
@clickhouse-gh clickhouse-gh Bot added the comp-keeper ClickHouse Keeper (Raft-based coordination layer; ZooKeeper replacement) and ZooKeeper client lib... label Sep 4, 2026
`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 antonio2368 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@kssenii
kssenii added this pull request to the merge queue Sep 11, 2026
Merged via the queue into master with commit 5827dc3 Sep 11, 2026
338 of 344 checks passed
@kssenii
kssenii deleted the keeper-backpressure-all-or-nothing branch September 11, 2026 09:21
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 11, 2026
@fm4v

fm4v commented Sep 14, 2026

Copy link
Copy Markdown
Member

@groeneai review keeper stress tests run results

@groeneai

Copy link
Copy Markdown
Collaborator

Keeper stress results for this PR

Keeper Stress Tests (PR) ran on all 5 heads (d413c0fc, f0dc53b8, 50a60eca, 04e75e26, 5d78884e), 3 no-fault scenarios per head (prod-mix, read-multi, write-multi; 900 s, 3 nodes, default backend). 15/15 scenario runs green: health_precheck, single_leader, error_rate_le, p99_le, log_sanity_ok, count_paths all passed, error_rate 0 in every run. One of the 87 other heads that ran the same job in the same window failed a gate, so the green here is a measurement and not a formality.

Final head against those 87 heads (keeper_stress_tests.keeper_metrics_ts):

scenario this PR, rps control p10 / p50 / p90, rps p99 write / read, ms
prod-mix-no-fault 6373 6330 / 6494 / 6562 566 / 484
read-multi-no-fault 173.8k 168.1k / 177.1k / 185.1k n/a / 10.3
write-multi-no-fault 8049 7981 / 8391 / 8706 978 / n/a

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 5827dc31, 09-11):

  • NightlyKeeperNoFaults 09-12 and 09-13: 10 scenarios each, all gates green, 0 errors; prod-mix 5654 and 5621 rps against 5483 (09-08) and 5537 (09-10) before the merge.
  • NightlyKeeperFaults 09-14, the first fault nightly carrying the change: 21 scenarios, all gates green, 0 errors. latency-slow-follower-fault 541 rps / write p99 27.3 s against 540 / 25.2 s, 538 / 25.8 s and 540 / 26.9 s on the three fault nightlies before it.
  • Keeper and Server Jepsen green every night 09-11 to 09-14; no keeper FAIL/ERROR row on master since the merge; test_keeper_slow_member_backpressure is 22880 OK / 0 not-OK over 1021 commits.

One limit worth naming: the stress suite never switches the feature on. bpon occurs in 5 files in the tree, none of them under tests/stress/keeper/, and zk_slow_member_backpressure is 0 in every mntr sample of every run (min = max = 0 over 9678 samples in the 09-14 fault nightly). So these runs measure the NuRaft bump and the off path; the on path is covered only by the new integration test. A scenario that sends bpon and then applies the existing slow-follower latency fault would cover it, which is a scenario plus a gate rather than new harness machinery. I can open that if you want it.

For completeness, the reds on the final head are not Keeper: the amd_msan AST fuzzer abort is the master-wide Can't determine table for parallel replicas family (my open fix is #113515), and the amd_tsan integration shard was test_storage_nats/test_nats_jet_stream.py plus a session timeout.

This branch was successfully deployed

1 active deployment
staging - docs — 5d78884e Deployed Sep 10, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-keeper ClickHouse Keeper (Raft-based coordination layer; ZooKeeper replacement) and ZooKeeper client lib... pr-feature Pull request with new product feature pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants