fix: send the first shard assignments right after the data server handshake - #1326
Merged
merlimat merged 3 commits intoSep 24, 2026
Merged
Conversation
…dshake Until the handshake binds a data server to the coordinator instance id, the data server rejects every coordinator RPC except the handshake and the health checks. The assignments dispatcher starts together with the health checks that perform the handshake, so it usually opened the stream first and got it rejected. It then retried only after the dispatch backoff: 10s initial, with jitter. On a fresh cluster, and for any data server not bound yet, the first assignments arrived 5-15s late, and clients spun on "server not initialized yet" in the meantime. Open the assignments stream only once the data server is Running or Draining. Status transitions now wake up the dispatcher, so it sends the first assignments right after the handshake, without retrying before it. A data server that failed its health checks is NotRunning too: the dispatcher now waits for it to come back and be handshaken again before reopening the stream, instead of retrying it on every backoff round. Failures on a Running data server still go through the regular backoff. With -race, TestCoordinator_ShrinkCluster drops from 12-23s to about 0.4s. Signed-off-by: Matteo Merli <mmerli@apache.org>
TestControlRequestFeatureEnabled and TestOrderedWrites read the shard leader from the coordinator metadata right after creating the client. Creating the client blocks until it receives the first shard assignments, which used to reach the data servers 5-15s after startup, long after the first leader election. Now that the assignments are sent right after the handshake, the client can connect before the election completes, and the tests read a nil leader. Wait for the shard to reach the steady state before looking up its leader. Signed-off-by: Matteo Merli <mmerli@apache.org>
Signed-off-by: Matteo Merli <mmerli@apache.org>
merlimat
requested review from
RobertIndie,
coderzc and
mattisonchao
as code owners
September 24, 2026 06:23
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Recovery after a stream ends while the server is not running can still be delayed by the backoff interval.
Review effort: Lite
Findings: None
What changed in this PR
This PR prevents delayed initial shard assignments by waiting for data-server handshake readiness.
Changes:
- Gates assignment streams on server readiness.
- Adds status-transition signaling and regression coverage.
- Tracks stream attempts in mocks.
- Updates control tests to wait for shard steady state.
| File | Summary |
|---|---|
tests/control/ordered_writes_test.go |
Waits for shard steady state. |
tests/control/control_request_test.go |
Waits for shard steady state. |
oxiad/coordinator/runtime/controller/mockutils/mock.go |
Tracks assignment stream attempts. |
oxiad/coordinator/runtime/controller/dataserver/dataserver_controller.go |
Gates streams on data-server readiness. |
oxiad/coordinator/runtime/controller/dataserver/dataserver_controller_test.go |
Tests immediate post-handshake assignment delivery. |
💡 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.
Motivation
On a fresh cluster, and for any data server that has not bound the coordinator instance id yet, data servers get their first shard assignments 5-15s late. Until then, clients connected to them keep retrying with
Failed receiving shard assignments, retrying later ... error="oxia: server not initialized yet".The data server rejects every coordinator RPC except
Handshakeand the health checks withErrNotInitializeduntil the handshake binds it (NewGrpcInsIDVerifyInterceptors). The data server controller starts the assignments dispatcher together with the health watch and ping goroutines that perform the handshake, so the dispatcher usually opensPushShardAssignmentsfirst. The stream is rejected: with gRPC, the rejection reaches the drain goroutine from #1216, and the dispatch loop sees the stream end ascontext.Canceled. The dispatcher then waits fordispatchAssignmentsBackoffbefore retrying: 10s initial, ±50% jitter.This is the assignments half of the startup handshake race. #1196 proposed gating on
Running || Draining, but it returned an error from the retry function, so it still waited for the backoff before the first retry.Changes
In
oxiad/coordinator/runtime/controller/dataserver/dataserver_controller.go:openAssignmentsStreamopens the stream only once the data server isRunningorDraining. It waits on astatusChangedchannel instead of polling, so the first assignments go out right after the handshake and nothing is sent before it.advanceStatusEpochLockedreplaces the threestatusEpoch++sites and closes and replacesstatusChanged, so every status transition wakes the waiters.Runningdata server still go through the regular 10s backoff.Behavior change: a data server that failed its health checks is
NotRunningtoo. The dispatcher now waits for it to come back and be handshaken again before reopening the stream, instead of retrying it on every backoff round. An unreachable node could not take the stream anyway. After a long outage, the next push now follows the recovery right away, instead of waiting for a backoff that has grown to its 60s cap (30-90s with jitter). ADrainingnode is never re-handshaken, so it is not gated.Test support: the mock RPC provider counts
PushShardAssignmentsattempts (PushShardAssignmentsCount).In
tests/control:TestControlRequestFeatureEnabledandTestOrderedWritesread the shard leader from the metadata right after creating the client. That only worked because client creation blocked on the late first assignments. They now wait for the shard's steady state first.This relies on #1325. The first snapshot the reconciler publishes has no leaders yet, because the first elections are still running, and main only hid that behind the delay:
NewShardManagerblocks until the first snapshot arrives. Without #1325, a client created right after startup would get that snapshot and fail its first requests to the empty leader with a non-retryableUnavailable. Before #1325,TestOIDCWithPerIssuerConfig,TestOIDCWithStaticKeyFileandTestControlRequestRecordChecksumfailed every run with this change. On top of #1325 they pass, and much faster than before. Go clients released before #1325 can still fail requests sent in the sub-second window before the first election on a fresh cluster. Main already exposes empty leaders the same way for namespaces created at runtime (per the code, not tested).Testing
TestDataServerController_SendsAssignmentsRightAfterHandshake: the mock rejects the stream withErrNotInitializedwhile the handshake is held in flight, with a 1-minute retry backoff. It fails on main (no assignments within 10s, because the retry is 30-90s away). With the fix it passes in ~0.2s, with exactly one stream opened, after the handshake. 50× repeat of the four dispatcher tests with-race: 200/200 pass.TestCoordinator_ShrinkClusterwith-race: 12.6-22.9s on main (5 runs), 0.33-0.56s with the fix (10 runs). Clientserver not initialized yetretries in the logs: 54 on main, 0 with the fix.go test -race ./oxiad/coordinator/...and everytests/package pass.-race, 3 runs each: both OIDC tests pass in 0.5-0.7s (6-14s before), and the threetests/controltests pass in 0.5-5.4s (10-20s before). The test commit also passes on the old main, 5/5 per test.oxiadandtestsmodules.