schema_registry: load _schemas once across shards on startup - #31137
Conversation
4653902 to
0e4165b
Compare
0e4165b to
714c62d
Compare
| } | ||
|
|
||
| ss::future<> service::do_start() { | ||
| if (_is_started) { |
There was a problem hiding this comment.
is it worth keeping _is_started around? maybe as an extra vassert(!_is_started) sanity check?
There was a problem hiding this comment.
I don't think so, but I don't feel strongly about it. Want me to re-add it?
There was a problem hiding this comment.
There is a difference in behavior related to one_shot. If the fetch fails, the one_shot will fail all waiters (transitively to the per-shard one_shot) and then rearm. A subsequent attempt could then retry and actually succeed. Don't have tests for that case, and do_start already retries, so this may not be important.
There was a problem hiding this comment.
ohhh sorry i see now the special one_shot typedef. I was missing that. SGTM
WillemKauf
left a comment
There was a problem hiding this comment.
LGTM, but i'd defer to @pgellert signing off on this too. Are there other SR experts we can start pulling into these reviews so we aren't overloading poor Gellert? 🙂
CI test resultstest results on build#87257
test results on build#87352
|
| "eager _schemas replay failed at start-up; will retry on " | ||
| "the first request: {}", |
There was a problem hiding this comment.
will retry on the first request
Is this bit true? The load once ensures that we will not retry reloading (which is fine), so I guess we may only "retry" in the sense that we will continue to try to replay the topic from the read syncs on the http handler path. Perhaps that's what you meant?
There was a problem hiding this comment.
If the fetch fails, the one_shot will fail all waiters (transitively to the per-shard one_shot) and then rearm. A subsequent attempt triggered by a request could then retry and actually succeed. Don't have tests for that case, and do_start already retries, so this may not be important.
@nguyen-andrew has done a bunch of work on SR, so feel free to throw him on reviews, too. Or anyone from the enterprise team really (@redpanda-data/core-enterprise, we're about to get everyone into this GH team) can do at least a single pass and bubble specific questions up to me. :) |
service is a per-shard peering_sharded_service, so each shard has its own _ensure_started one_shot. do_start() only stamps _is_started (its re-entry guard) from inside its invoke_on_all, after create_internal_topic(), so first requests arriving on several shards concurrently each run do_start() and launch an independent, redundant full replay of the _schemas topic on the reader shard - slowing recovery by up to the shard count. Funnel every shard through a single one_shot on the reader shard so the topic is loaded exactly once. The per-shard one_shot still caches completion, so only the first request on each shard pays a cross-shard hop; failure/retry semantics are unchanged. Covered by a ducktape test that restarts the registry under concurrent multi-shard load and asserts exactly one replay runs per restart. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_load_once makes do_start() run exactly once on the reader shard, so its old cross-shard re-entry guard is redundant. Drop the _is_started flag, assert do_start() runs on the reader shard, and run the fetch inline rather than via invoke_on_all (whose only remaining job was setting _is_started on every shard). Removing _is_started also fixes a latent case: a fetch that exhausted its retries left _is_started set, so the _load_once retry short-circuited and the store never loaded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
714c62d to
a553561
Compare
| *this, | ||
| "schema_registry_replay_on_startup", | ||
| "Replay the internal `_schemas` topic into the Schema Registry store at " | ||
| "broker start-up instead of lazily on the first request. Makes recovery " |
There was a problem hiding this comment.
nit: I think instead of "at broker start-up", we could say "at Schema Registry start-up", since I believe we can restart the schema registry service (and have this new config take effect) without having to restart the broker
There was a problem hiding this comment.
Good point, fixed.
Recovery of the _schemas store is lazy: nothing loads the topic until the first request (or an internal schema::registry access) reaches ensure_started(). On a large registry the first request then blocks for the whole replay, and there is no way to hydrate the store ahead of traffic. Add a cluster config (default off) that, when set, proactively drives the replay on the reader shard at service start-up, fire-and-forget under the gate so it does not block broker start-up. It reuses the single-flight ensure_started() path, so a request that races the eager trigger still results in exactly one replay. Covered by ducktape tests for both settings: with the config set the store hydrates on restart with no client request, and with it unset recovery stays lazy until the first request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a553561 to
e9aae73
Compare
|
/backport v26.2.x |
|
/backport v26.1.x |
|
Branch name "v26.2.x" not found. |
|
/backport v26.2.x |
Schema Registry recovery of the internal
_schemastopic on startup couldrun multiple redundant full replays concurrently on the reader shard, making
cold start of a large registry take much longer than necessary (recovery
time scaled with the number of racing shards).
serviceis a per-shardpeering_sharded_service, so each shard owns itsown
_ensure_startedone_shot, anddo_start()only stamps_is_started(its re-entry guard) from inside its
invoke_on_all, aftercreate_internal_topic(). Previously, when first requests landed on severalshards within that window, each shard ran its own
do_start()and launchedan independent replay.
This PR instead funnels every shard through a single one_shot on the reader
shard so the topic is loaded exactly once; the per-shard one_shot still
caches completion, so only the first request on each shard pays a
cross-shard hop and steady state is unchanged.
Recovery is also lazy (nothing loads the topic until the first request), so
the first request after a restart can block for the whole replay. This PR
adds a cluster config,
schema_registry_replay_on_startup(default off),that proactively drives the single-flight load at startup, fire-and-forget
under the gate, so the store hydrates ahead of traffic without blocking
broker start-up.
Covered by ducktape tests: a single replay per restart under concurrent
multi-shard load, and the eager/lazy behavior of the new config.
Backports Required
Release Notes
Improvements
_schemastopic exactly oncewhen recovering on startup, instead of running redundant concurrent
replays; cold start of a large registry is significantly faster.
schema_registry_replay_on_startupcluster property (default off)that hydrates the Schema Registry store at broker start-up rather than
lazily on the first request.