Repository navigation
test: fix the end-to-end benchmark setup when a member answers the election late - #1454
Merged
Merged
Conversation
…ection late waitForFollowersApplied classified the servers once, right after the keys were loaded, and took any server with a leader controller as the leader. But NewTerm creates a fenced leader controller on every member that isn't a follower yet, and a member only becomes a follower when the leader's cursor reaches it. A member that answers NewTerm after the coordinator's 100ms grace period is left out of the election and is added back only when the coordinator retries it, so the loaded puts can complete on the leader and the other follower first. Two servers then answered GetLeader, and the setup failed with "should have 2 item(s), but has 1" in about 1 of 180 cluster setups, which also stopped dev/bench-compare.sh before benchstat. Take the leader to be the server whose leader controller is LEADER, and wait in the Eventually until every other server has a follower controller that has applied the leader's commit offset. Signed-off-by: Matteo Merli <mmerli@apache.org>
merlimat
requested review from
RobertIndie,
coderzc and
mattisonchao
as code owners
October 1, 2026 17:53
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused benchmark-only change correctly handles fenced controllers and late follower creation.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes flaky end-to-end benchmark setup when an election participant responds late.
Changes:
- Identifies the active leader by
LEADERstatus. - Polls dynamically until every other server becomes a caught-up follower.
| File | Description |
|---|---|
oxiad/dataserver/e2e_benchmark_test.go |
Makes follower synchronization resilient to late election responses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
Before the timed run,
BenchmarkE2E(#1443, #1445) waits for the followers to apply the loaded keys.waitForFollowersApplieddecides who leads and who follows only once, right after the 10k loading puts, and it treats any server whereGetLeadersucceeds as the leader.However,
NewTermcreates a fenced leader controller on every member that isn't a follower yet. A member only becomes a follower when the leader's cursor reaches it. If a member answersNewTermafter the coordinator's 100 ms grace period (quorumFencingGracePeriod), it is left out of the election. It is added back only when the coordinator retries it (fencingFailedFollowers). The loading puts need only the leader and the other follower, so they can complete before that happens. Two servers then answerGetLeader, and the setup fails.On main this happens in about 1 of 180 cluster setups (
-test.benchtime 1x -test.count 60). When it happens duringdev/bench-compare.sh,set -estops the whole comparison before benchstat.Example
NewTermright away. s2 creates its fenced leader controller, but its answer arrives after the 100 ms grace period.Before:
GetLeadersucceeds on s0 and also on s2 (its fenced controller). Only s1 is counted as a follower, so the setup fails:After:
LEADER, which is s0. s2's controller isFENCED.Modification
Status() == proto.ServingStatus_LEADER, not byGetLeadersucceeding. Its commit offset is still read before the extra put, because followers learn the commit offset only from the next append.require.Eventually: every server other than the leader must have a follower controller (GetFollowersucceeds) that has applied that offset. This replaces therequire.Lenon a follower list that was built once, up front.Verification
NewTerm1 s late, after creating its fenced leader controller.-test.count 120) on b96d9c8 and 30 more on 1ea2328 had 0 failures.dev/bench-compare.shruns to completion and prints the benchstat tables../oxiad/....