Repository navigation
adapter: Anchor 0dt stability on replica-reported hydration - #39152
Conversation
|
Reply to your pending review question on the status clone in message_handler.rs: This updates health evidence even when the visible status and restart count are unchanged. A pod can go Ready → NotReady → Ready across a watch reconnect without restarting its containers. The coordinator sees Online both times and the same restart count, but the new Ready transition gives a later healthy_since. Without this update, we would keep counting the unhealthy interval as continuous health. Cloning the old record preserves its status-change time: rewriting that with every event timestamp would make ordinary watch updates look like flaps. Only healthy_since changes here. The comment above this branch should explain that distinction more clearly. Posting here because GitHub rejected an inline reply while your review is pending. I have left your review unsubmitted. |
3b36eaf to
0f3fa7b
Compare
6203f3e to
b156146
Compare
b0d9af6 to
cd91eb1
Compare
|
@ggevay replies to your two review comments, since inline replies are blocked on my side: On period counting from pod readiness: you're right, that was a real regression. The streak was seeded from pod readiness, so the post-hydration observation window disappeared. The gate now anchors on when each replica reports having hydrated the cluster's gated collections, read from its hydration log. So readiness is back to hydration plus the period, and it still survives environmentd restarts. On the |
cd91eb1 to
f064f10
Compare
| // rewriting the record when neither the status nor the restart count | ||
| // changed. | ||
| if !status_changed && !restart_count_changed { | ||
| let mut status = old_process_status.clone(); |
There was a problem hiding this comment.
why do we have this one in here?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Outdated: that restart-count record rewriting was part of the orchestrator anchors, which the rework removed. The only change left in message_handler.rs is the CaughtUpCheck dispatch.
| "enable_0dt_caught_up_stability_check", | ||
| true, | ||
| "Require clusters to stay caught-up and healthy for a stability period before being considered ready during 0dt deployments. Emergency break-glass flag: disabling reverts to treating a caught-up cluster as ready with no replica-health requirement, which differs from setting the stability period to zero (a zero period still requires all replicas to be healthy).", | ||
| "Require caught-up clusters to have had every replica online and hydrated for a stability period before being considered ready during 0dt deployments. Emergency break-glass flag: disabling reverts to treating a caught-up cluster as ready with no replica-health requirement, which differs from setting the stability period to zero (a zero period still requires all replicas to be online and hydrated).", |
There was a problem hiding this comment.
what's up with these dyncfg changes, should roll back because they don't add anything
There was a problem hiding this comment.
AJ (Aljoscha's agent): Agreed, reverted. Both descriptions are back to what's on main.
|
|
||
| assert ( | ||
| _pids(c, "mz_new", "environmentd") != environmentd | ||
| ), "the DDL did not restart mz_new" |
There was a problem hiding this comment.
is this actually testing what we want to test then, the problem did only previously occur when envd actually had to restart, no?
There was a problem hiding this comment.
AJ (Aljoscha's agent): It does test the restart. It asserts that the environmentd pid changed after the CREATE TABLE, so the DDL really rebooted mz_new, and that the clusterd pids did not. Then it asserts mz_new became ready before ddl_after + period, which a period restarted by the reboot could not meet. It passed in Zero downtime 2 on nightly 18390, and the pid assert would have failed without a restart.
| //! caught-up cluster is ready only once every replica is `Online` and has had | ||
| //! the cluster's gated compute collections hydrated for a configurable period. | ||
| //! | ||
| //! The gate learns when a replica hydrated by querying that replica's |
There was a problem hiding this comment.
please lose the new gate language here, sounds quite clichee
There was a problem hiding this comment.
AJ (Aljoscha's agent): Reworded. The module doc now just says where hydration times come from.
| /// Reads the log's index directly, so the query stays on the fast path instead | ||
| /// of installing a dataflow on the replica for every check. The per-worker | ||
| /// aggregation happens in [`replica_hydrated_since`]. | ||
| const HYDRATION_QUERY: &str = "SELECT export_id, hydrated_at \ |
There was a problem hiding this comment.
are we sure there is an index? Either way running the query is fine, but we might want to skip jabbering about and index here and just describe the query, yah?
There was a problem hiding this comment.
AJ (Aljoscha's agent): There is one: every introspection log is maintained as an arrangement on each replica with logging, which is what makes this a fast-path peek. But agreed, the doc now just describes what the query returns.
| } | ||
|
|
||
| /// Per-cluster state for the stability gate, retained across caught-up checks. | ||
| /// An uninterrupted run of `Online` observations of one replica by this |
There was a problem hiding this comment.
Do you know when we actually are in that situation? If this is only a thing that can happen in testing, we can remove the complexity and just not do the local streak thing, don't do the check for these replicas.
There was a problem hiding this comment.
AJ (Aljoscha's agent): It is not test-only: users can turn introspection off (INTROSPECTION INTERVAL = 0). But the local streak reset on every environmentd restart, which is exactly what this PR fixes, so I removed it as you suggested. Such replicas only need to be Online now.
| enum ReplicaHealth { | ||
| /// The replica is not currently `Online`. | ||
| NotOnline, | ||
| /// The replica is `Online`, but has not hydrated every gated collection, |
There was a problem hiding this comment.
asked elsewhere, why do we care about the specific "gated collections"?
There was a problem hiding this comment.
AJ (Aljoscha's agent): We don't anymore, see the query thread.
| /// or did not answer the hydration query. | ||
| NotHydrated, | ||
| /// The replica has been healthy since this wall-clock time. | ||
| Since(EpochMillis), |
There was a problem hiding this comment.
Maybe HealthySince(...)?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Done, HealthySince.
| } | ||
|
|
||
| /// Outcome of folding one health snapshot into a [`ClusterStabilityState`]. | ||
| /// The stability gate's verdict for one cluster on a given tick. |
There was a problem hiding this comment.
we need the rust doc change here?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Only because the old doc linked ClusterStabilityState, which no longer exists. I kept the rewording minimal.
| return all_caught_up; | ||
| } | ||
|
|
||
| let mut all_stable = true; |
There was a problem hiding this comment.
potentially, when we're not even all caught up, we can skip the stability check no?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Yes, done. With the local streak gone there is no state to maintain, so the task skips the queries entirely while some cluster is not caught up.
1778649 to
4c6155b
Compare
The 0dt stability gate kept its healthy streak in environmentd memory, so every DDL-triggered reboot of the read-only environmentd restarted the stability period, even though the incoming replicas kept running. With frequent DDL a deployment could never become ready. Ask the replicas instead. Each tick queries every replica's mz_compute_hydration_times_per_worker log for the latest hydration it reports, or NULL if anything it hosts is not hydrated yet. Compute collections the caught-up classification ignored are left out of that query. A caught-up cluster is ready once every replica is Online and has been fully hydrated for the stability period. The log lives in the replica process: a restarted environmentd reconciles with the same processes and sees the original hydration times, while a restarted replica has to hydrate again. The queries go through the frontend peek path, which calls back into the coordinator, so the check moves into a task of its own. Each tick it asks the coordinator to classify the clusters and then applies the stability gate itself. PeekClient::background_peek runs a coordinator-owned SELECT pinned to a replica, and the background PeekClient constructor is shared with hydration history collection. Replicas are asked concurrently, each with a timeout, and a query that misses it is dropped. Replicas without introspection logging cannot answer and only need to be Online. This replaces the per-cluster flap and restart tracking, which replica-reported hydration makes redundant. Closes: SQL-691
|
@ggevay this now has the reworked semantics |
4c6155b to
66e345c
Compare
ggevay
left a comment
There was a problem hiding this comment.
Looks good to me. One description nit: "The period counts from hydration, as before" isn't quite right, since on main the period starts at the first tick where the cluster is caught up and healthy. Maybe say that hydration is a proxy for that, and that a long catch-up after hydration eats into the period.
| replicas = _pids(c, "mz_new", "clusterd") | ||
|
|
||
| # A table has no dataflow, so this changes no cluster's gated collections. | ||
| c.sql("CREATE TABLE unrelated (a int)", service="mz_old") |
There was a problem hiding this comment.
With the default 5 min with_0dt_deployment_ddl_check_interval, only the final check before announcing readiness sees this CREATE TABLE. So the restart comes at the end of the period rather than partway through, and the reboot sits on the critical path (Nightly 18393: halt at the end of the period, ready after 143 s against the 180 s bound). Could the test set the interval to 1 s next to the period, like ddl_detection_ephemeral_items does, and assert ready < restart + period from the environmentd pid change?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Agreed, done in 5d3343c. The test now sets with_0dt_deployment_ddl_check_interval = 1s next to the period, waits for the environmentd pid to change after the CREATE TABLE, and asserts mz_new is ready less than one period after that restart.
| // fleet-wide switch. Operators can force a single cutover via | ||
| // skip-catchup/promote, but this flag changes auto-cutover across all | ||
| // environments without per-deploy manual intervention or a code | ||
| // release, mirroring `enable_0dt_caught_up_replica_status_check`. |
There was a problem hiding this comment.
AJ (Aljoscha's agent): Trimmed to two sentences in 5d3343c.
| NotHydrated, | ||
| /// The replica has been healthy since this wall-clock time. | ||
| HealthySince(EpochMillis), | ||
| /// The replica is `Online` and cannot report hydration times, so there is |
There was a problem hiding this comment.
Maybe change the "and" to "but"
| } | ||
|
|
||
| /// How a cluster relates to the 0dt caught-up check on a given tick. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| enum ClusterCaughtUpStatus { | ||
| /// Genuinely hydrated and within lag. Subject to the stability gate. |
There was a problem hiding this comment.
Maybe
/// Hydrated and within lag, apart from `ignored_compute_collections`.
/// Subject to the stability gate.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Taken as suggested in 5d3343c.
| //! caught-up cluster is ready only once every replica is `Online` and has had | ||
| //! everything it hosts hydrated for a configurable period. |
There was a problem hiding this comment.
The comment at L459-461 ("We care about each collection being hydrated on some replica. We don't check that at least one replica has all collections of that cluster hydrated.") is still true for the classification, but now reads like the overall rule, while the gate requires every replica to have hydrated everything it hosts. Maybe scope it to the classification?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Scoped it in 5d3343c: it now says the classification only needs each collection hydrated on some replica, and that the stability gate separately requires every replica to have hydrated everything it hosts.
| environmentd = _pids(c, "mz_new", "environmentd") | ||
| replicas = _pids(c, "mz_new", "clusterd") | ||
|
|
||
| # A table has no dataflow, so this changes no cluster's gated collections. |
There was a problem hiding this comment.
Nit, "gated collections" no longer exists:
| # A table has no dataflow, so this changes no cluster's gated collections. | |
| # A table has no dataflow, so no replica gets a new export to hydrate. |
There was a problem hiding this comment.
AJ (Aljoscha's agent): Applied in 5d3343c.
| //! A replica without introspection logging cannot report hydration times. It | ||
| //! only has to be `Online`. |
There was a problem hiding this comment.
Could we add a NOTE at ComputeReplicaLogging::interval (compute-types/src/config.rs), where None (from INTROSPECTION INTERVAL = 0) disables logging? E.g.: "NOTE: 0dt cutovers measure the stability period from this log. With logging disabled they still wait for hydration, but not for the period, so a crash loop can pass." That way whoever turns introspection off sees the cost.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Added in 5d3343c, close to your wording, with a pointer to mz_adapter::coord::caught_up.
66e345c to
5d3343c
Compare
|
AJ (Aljoscha's agent): @ggevay thanks for the review. Fixed the description nit: it now says main starts the period at the first tick where the cluster is caught up and healthy, that hydration is a proxy for that, and that a long catch-up after hydration eats into the period. The inline nits are addressed in 5d3343c, replies on each thread. |
antiguru
left a comment
There was a problem hiding this comment.
Posted by Claude Code on behalf of @antiguru.
No blockers found. Inline comments cover the main points. Smaller notes:
- Requiring every replica to hydrate means one undersized replica now blocks promotion until timeout or skip-catchup. Worth mentioning in the release notes, since operators will see it as a stuck deploy.
hydrated_atcomes from the replica clock, so a lagging replica clock shortens the period by its skew. The previous gate compared only orchestrator times against each other.background_peekaccepts arbitrary SQL and relies on the doc comment to keep user input out. A typed query or a narrower signature would enforce that.
| for replica in cluster.replicas { | ||
| let replica_health = match replica { | ||
| ReplicaTarget { online: false, .. } => ReplicaHealth::NotOnline, | ||
| ReplicaTarget { logging: false, .. } => ReplicaHealth::Exempt, |
There was a problem hiding this comment.
Replicas with introspection disabled lose crash-loop protection entirely: before this PR the restart-count and status-flap tracking caught a crash-looping replica regardless of logging, now Online at a single tick suffices. Could Exempt replicas keep the previous in-memory restart-count streak? It resets on environmentd restart, but only for this subset, which beats no gate.
There was a problem hiding this comment.
AJ (Aljoscha's agent): We are keeping this as is. The in-memory streak resets on exactly the environmentd restarts this PR is about, and keeping it for this subset would mean two anchors and two code paths. Turning introspection off is an opt-in, and the NOTE ggevay asked for at ComputeReplicaLogging::interval now states the cost where someone turns it off.
| /// with the coordinator and its issue to the replica would orphan the | ||
| /// registration. | ||
| /// | ||
| /// NOTE: Nothing cancels a timed-out peek, since there is no connection to |
There was a problem hiding this comment.
With the caught-up task, an Online replica that is slow to answer gets a fresh peek every tick (10s default), and each one keeps read holds on its log indexes until it answers. That holds back log compaction on that replica for as long as the 0dt wait lasts. Could the gate skip a replica with an outstanding peek, or cancel through the coordinator on timeout?
There was a problem hiding this comment.
AJ (Aljoscha's agent): Done in df88dec, together with the concurrency point. A query that misses its deadline keeps running in its own task (dropping it could orphan its peek registration), and its replica is not asked again until it finishes, so a slow replica holds at most one peek. background_peek no longer takes a timeout, the gate owns the deadline.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Update: reworked in ee05334 to cancel instead. Each background peek now runs on a connection ID of its own, from the allocator the coordinator shares with Client, so on timeout background_peek sends the regular PrivilegedCancelRequest for it, which cancels the peek at the controller and releases its read holds. No query outlives its tick.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Correction to my last reply: the per-connection cancel crashed environmentd in a nightly (PrivilegedCancelRequest expects a registered session), so ff69edd replaces it. Frontend fast-path peeks are now cancel-safe: a guard armed before registration retires the registration and cancels the peek on its instance when the future or the response stream is dropped before the response. The gate just wraps each query in a timeout.
| let mut all_ready = true; | ||
| for (cluster_id, cluster) in caught_up_clusters { | ||
| let mut health = BTreeMap::new(); | ||
| for replica in cluster.replicas { |
There was a problem hiding this comment.
Replicas are queried sequentially, so a tick costs up to N x 5s plus planning, while now is taken before the loop. Conservative, but slow with many replicas. Running the queries concurrently (e.g. join_all per cluster or across clusters) would bound a tick by the slowest replica.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Done in df88dec. All replicas are asked concurrently against one shared 5s deadline, so a tick is bounded by the slowest replica. Only answers to queries issued in that tick are used.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Update: still concurrent with one 5s timeout per query, but timed-out queries are now canceled (see the read-hold thread), so each tick starts clean.
| return ReplicaHealth::NotHydrated; | ||
| } | ||
| let millis = hydrated_at.unwrap_timestamptz().timestamp_millis(); | ||
| ReplicaHealth::HealthySince(u64::try_from(millis).unwrap_or(0)) |
There was a problem hiding this comment.
A negative hydrated_at maps to 0, which reads as stable for decades and makes the cluster ready immediately. Unlikely, but the fallback should fail closed, i.e. ReplicaHealth::NotHydrated.
There was a problem hiding this comment.
AJ (Aljoscha's agent): Agreed, fixed in df88dec: a pre-epoch hydrated_at now reads as NotHydrated, with a unit test next to the NULL and no-row cases.
5d3343c to
df88dec
Compare
|
AJ (Aljoscha's agent): @antiguru thanks for the review. On the smaller notes:
|
6e2cb88 to
f631449
Compare
Motivation
Closes: SQL-691
A DDL-triggered reboot of the read-only environmentd discarded the 0dt stability streak, even when the incoming replicas kept running. With DDL more frequent than the stability period, a deployment could never become ready.
Description
The stability gate now asks the replicas. Each tick queries every replica's
mz_compute_hydration_times_per_workerlog with a one-row query: the latesthydrated_atit reports, or NULL if any non-transient export on it is not hydrated yet. Compute collections the caught-up classification ignored (excluded new builtins and their dependents, collections whose leader frontier is past the cutoff) are left out of the query, because they may never hydrate in read-only mode. A caught-up cluster is ready once every replica is Online and has been fully hydrated for the stability period. A replica-pinned MV or a collection without a dataflow never appears in a replica's log, so neither needs special handling.The queries are plain SQL through the frontend peek path (
PeekClient::background_peek), at serializable isolation so they read the freshest log state even when the timestamp oracle does not advance. The frontend peek path calls back into the coordinator, so the check now runs in its own task, like the cluster controller: each tick it asks the coordinator to classify the clusters, then queries the replicas and applies the gate itself. The backgroundPeekClientconstructor is shared with hydration history collection. Replicas are asked concurrently, so a tick waits at most 5s for answers. A query that misses the deadline is dropped. That does not cancel its peek: it keeps its read holds until the replica answers, and in the rare case of a drop between registration and issue the registration stays behind. We accept that, like for user peeks: a stuck replica holds at most one peek per tick, on a log index that barely changes.The log lives in the replica process. A restarted environmentd reconciles with the same replica processes, which keep compatible dataflows, so it reads the original hydration times. A restarted replica starts a new log and must hydrate again. Hydration times come from the replica's clock: a replica clock ahead of environmentd's only lengthens the wait, one behind shortens the period by the skew. The period counts from the replica's latest hydration. On main it starts at the first tick where the cluster is caught up and healthy. Hydration is a proxy for that, so a long catch-up after hydration eats into the period.
Replicas without introspection logging cannot answer and only need to be Online. The per-cluster status-flap and restart tracking is removed, because replica-reported hydration covers restarts directly. No orchestrator changes and no new durable state.
Behavior changes worth noting:
with_0dt_deployment_max_wait. adapter: Compare 0dt hydration against the leader #39155 relaxes this for replicas the leader reports unhydrated too.Verification
Unit tests cover the stability fold and that a missing, NULL, or pre-epoch
hydrated_atreads as not hydrated. The newcaught_up_stability_survives_restart0dt workflow triggers a DDL restart of mz_new partway through the stability period, checks that its replicas survived, and asserts it becomes ready before a restarted period could end. The newcaught_up_stability_without_dataflowsworkflow checks that a sealedREFRESH ATMV and a replica-pinned MV on a two-replica cluster do not block promotion. The existingcaught_up_stabilityandcaught_up_stability_crash_loopworkflows cover the healthy and crash-looping cases.Release notes
with_0dt_deployment_max_waitpasses.