Repository navigation
adapter: Move 0dt catch-up waits off the coordinator - #39710
Conversation
Read leader state before capturing local controller evidence. Keep the coordinator handler synchronous and batch hydration requests per cluster. Preserve cutoff exemptions without waiting for hydration, and reject missing required evidence.
An overdue interval tick fires immediately even with Skip, so slow reads can run continuously. Sleep between attempts, including retries, while keeping the first check immediate.
|
I've triggered the relevant subset of Nightly: https://buildkite.com/materialize/nightly/builds/18504 |
| let period = WITH_0DT_DEPLOYMENT_CAUGHT_UP_CHECK_INTERVAL | ||
| .get(self.catalog().system_config().dyncfgs()); | ||
| let internal_cmd_tx = self.internal_cmd_tx.clone(); | ||
| let mut catalog = self.owned_catalog(); |
There was a problem hiding this comment.
it work right now to store this catalog, because we assume it will not change, but that sounds a bit brittle. Maybe we should still call into the coordinator to get a fresh catalog snapshot each tick?
There was a problem hiding this comment.
ahh, I see below we are doing this, right?
There was a problem hiding this comment.
Looks good, this fixes SQL-806, and also turns SQL-768's panic into a retried warning. Two minor comments. Let's wait for the Nightly before merging.
For the description: the impact in today's incident was more than delayed work. Once a check took longer than the interval, the coordinator almost never polled cluster events, because #39152 made the check an internal message, the first branch of the biased select. The Kubernetes pod watch never finished its initial list, every replica stayed NotOnline, and the stability gate could not pass.
| @@ -471,6 +505,16 @@ impl Coordinator { | |||
| interval.set_missed_tick_behavior(MissedTickBehavior::Skip); | |||
There was a problem hiding this comment.
When a check takes longer than the interval, Skip still fires the overdue tick immediately (it only realigns the next deadline), so checks run back to back. In today's incident a check took 18 s and growing, because the leader's mz_cluster_replica_frontiers shard had grown to ~160M updates. With this PR that no longer blocks the coordinator, but the task would then read the leader's shards continuously. Maybe sleep a full interval after each check instead?
| }; | ||
| if gate.ready(snapshot).await { | ||
| catalog = Arc::clone(&check.catalog); | ||
| if catalog.transient_revision() != leader_revision { |
There was a problem hiding this comment.
I think these revision checks can't fire while read-only: transient_revision only changes in Catalog::transact, whose only non-test caller, catalog_transact_inner, returns AdapterError::ReadOnly first, and leader DDL reaches the read-only envd as a halt from check_ddl_changes. If they're meant for a future where the read-only envd applies DDL in place, could the comment say so? Otherwise I'd drop them.
There was a problem hiding this comment.
yeah, this was "abundance of caution" 😅
2 participants
Motivation
Closes SQL-806
Closes SQL-768
The read-only coordinator awaits Persist reads and one compute-controller request per collection during each 0dt caught-up check. Slow reads stall its message loop and delay other work in the incoming deployment.
Description
Move the reads, classification and hydration waits into the existing caught-up task. Read the leader before capturing local frontiers and hydration, so slow Persist reads do not age local evidence or introduce artificial lag. The coordinator only captures local state and issues one hydration request per cluster.
Keep the existing readiness and exemption rules. Required read failures and lost replies leave the deployment unready, while ignored collections need no hydration reply. Discard checks invalidated by catalog changes.
Verification
Add a server regression that holds a catch-up read and requires the incoming coordinator to answer a SQL query. Add unit coverage for pending, missing and dropped hydration evidence, and for cutoff exemptions that must not wait for hydration.
Release notes
None.