adapter: delete the staged cluster reconfiguration and direct replica paths - #38103
Conversation
385e39c to
281b832
Compare
281b832 to
6aacdb9
Compare
| } | ||
| } | ||
| if let Some(target) = folded_target { | ||
| let Managed(target_managed) = &mut new_config.variant else { |
There was a problem hiding this comment.
should do an exhaustive destructure here please
There was a problem hiding this comment.
Done. Destructures ReconfigurationTarget so a new target dimension has to be applied here too.
| use mz_sql::plan::AlterOptionParameter::Unchanged; | ||
|
|
||
| let new_target = ReconfigurationTarget { | ||
| size: new_managed.size.clone(), |
There was a problem hiding this comment.
prolly also want exhaustive destructure here
There was a problem hiding this comment.
Done, and I did options as well as new_managed since it carries the same hazard, arguably the sharper one: a new ALTER option that names a shape dimension has to be either folded or explicitly ruled out.
The non-shape fields are spelled out with _ and a one-line reason rather than .., otherwise the destructure buys nothing. This matches alter_changes_replica_shape a few lines up, which already destructured PlanClusterOption exhaustively.
Verified it actually bites rather than just looking exhaustive: adding a probe_new_shape_dimension field to PlanClusterOption yields
error[E0027]: pattern does not mention field `probe_new_shape_dimension`
--> src/adapter/src/coord/sequencer/inner/cluster.rs:2259:9
at the new site (and at alter_changes_replica_shape). Reverted the probe, of course.
6aacdb9 to
0c59e5b
Compare
| let state = self | ||
| .observe_cluster_state(cluster_id) | ||
| .expect("managed cluster observed above"); | ||
| reconcile_replicas(&state, &[desired]) |
There was a problem hiding this comment.
One thing I am not quite understanding here...
My understanding was that the controller passes one contribution per strategy into as the desired state (including HydrationBurst). Since a burst replica is owned and has a different size, will reconiciliation drop the burst replica if the wait for is 0s?
If so, would that leave the burst record in place?
Was there silent guarding of this behaviour on the old code path before, with the full reconciliation?
I see some test changes as well in similar scenarios, but not quite sure ...
Either way flagging it.
There was a problem hiding this comment.
You found a bug! This was somewhat working before, and even is now, but it's tricky:
- Before, even a wait = 0s, commit cutover would go through the controller, and it would do the correct thing.
- I decided to keep this non-controller path for wait = 0s, commit, as a failsafe, but it's complicating things slightly because it duplicates the decision logic, and it did it incorrectly 🙈
What would happen right now: we drop the burst replica, we keep the burst record. On the next controller tick the burst replica is re-created, but we have lost it's state, of course.
I'm slightly leaning towards keeping this reconfiguration path, and making sure it works correctly by re-using the same decision code. But I could probably be convinced to remove it, and let the controller handle the wait =0s, commit path as well. Wdyt?
There was a problem hiding this comment.
:) Agreed on keeping it! I think if we can somehow refactor to maximize code reuse across the code path that is good as well.
e115a22 to
86f1766
Compare
|
Heads up: #38139 (just merged) adds a "single-replica sources on a multi-replica cluster" notice with emission points inside
One non-conflict to also pick up: the doc comment on |
| /// | ||
| /// A zero timeout that rolls back is *not* this: it asks for the reconfiguration | ||
| /// to be abandoned at once, which is the record path's job. | ||
| fn requests_immediate_cut_over(strategy: &AlterClusterPlanStrategy) -> bool { |
There was a problem hiding this comment.
The AlterClusterReplicationFactorWhileReconfiguring refusal still fires for the immediate cut-over, so the escape hatch can't change the factor while unsticking a wedged record (the new test's factor case has no record in flight). The refusal's rationale, silent clobber at the async cut-over, doesn't apply here: the cut-over retires the record in the same transaction, and nothing consumes a retired record's target. Consider exempting requests_immediate_cut_over, or noting the two-step workaround (cut over the shape first, then alter the factor) in the refusal's comment.
There was a problem hiding this comment.
Good catch, fixed in #38174 (stacked on top, since #38112 is mid-stack and rewrites the neighbouring dimension comments).
Your reasoning checks out end to end: fold_reconfiguration_target already computes the right answer for a re-targeted factor, the cut-over retires the record in the same transaction, and every consumer of a record's target sits behind is_in_progress() — so there's no later cut-over left to clobber the write. Only the guard made that branch unreachable.
Went with exempting rather than documenting the workaround, because the two-step version makes the cluster run the old factor at the new size in between, which for a scale-down is the most expensive combination available, exactly while someone is firefighting.
Kept it narrow in one respect: a zero-timeout rollback still leaves a record in flight to settle on a tick, so it keeps the refusal. And you were right about why this held — the existing size-and-factor case runs with no record in flight, so the wedged case was simply unexercised. #38174 adds it, plus an assertion that the refusal still fires without a WAIT.
The wider question your comment raises — should this be allowed generally, not just on the cut-over — I've kept out of that PR. Worth noting the surface is further along than it looks: the factor is already re-targetable at record creation, and mz_cluster_reconfigurations.changes and the SHOW CLUSTERS summary already render replication factor to N. So only re-targeting an existing record is refused. But it's a user-visible semantics change on a surface about to go GA, so it's getting tracked separately.
There was a problem hiding this comment.
Filed the general-semantics question as SQL-626, in the Cluster Autoscaling on hydration project alongside the rest of this work, related to SQL-342. It writes up the three designs (fold into the record / apply immediately and sync the target / drop the factor from ReconfigurationTarget entirely) and what each costs the user, including the combined-statement overlap-peak change, which is billing-visible.
| let mut final_config = ClusterConfig { | ||
| variant: ClusterVariant::Managed(new_config), | ||
| workload_class: workload_class.clone(), | ||
| }; |
There was a problem hiding this comment.
nit: this was the last producer of Op::UpdateClusterReplicaConfig, so the op variant, its transact.rs apply arm, and the ddl.rs audit match arm can go with the machine too (here or in a follow-up).
There was a problem hiding this comment.
Agreed, done in #38174. Checked there are no producers left anywhere in src/ or test/ — the variant, the transact.rs apply arm and the ddl.rs audit match arm all went.
def-
left a comment
def-
left a comment
There was a problem hiding this comment.
How do we plan to handle the scenario where the new small replica never finishes hydrating? It would be nice if we could handle that gracefully. We have a temporary quick-hydration replica that stays around during that scenario, but if you run ALTER CLUSTER ... SET (SIZE '...') WITH (WAIT FOR '0s') the temp replica also gets destroyed, and then immediately recreates the temp replica again, destroying its state. Small test:
diff --git a/test/testdrive/cluster-controller.td b/test/testdrive/cluster-controller.td
index e2d0f236b5..afab201939 100644
--- a/test/testdrive/cluster-controller.td
+++ b/test/testdrive/cluster-controller.td
@@ -1449,6 +1449,78 @@ scale=1,workers=2 2
> DROP CLUSTER cc_cutover CASCADE
> DROP TABLE cc_cutover_t
+# ----- Direct cut-over: a running hydration burst survives -----
+#
+# The cut-over converges the replica set with the same reconcile kernel a tick
+# does, and that kernel unions *every* strategy's contribution, so a replica
+# survives iff some strategy still desires its shape. The cut-over supersedes
+# the reconfiguration record but not the burst record beside it, which the
+# reshape leaves in place and warranted, so the burst's contribution belongs in
+# that union too. Drop it and the same reshape churns the replica set
+# differently depending on which path ran it: the next tick puts the burst
+# replica back under a fresh id, discarding its hydration progress.
+#
+# Same wedge as above: a sleeping view pins hydration on every replica, which
+# both arms the burst and keeps the reconfiguration from cutting over by itself.
+
+$ postgres-execute connection=postgres://mz_system@${testdrive.materialize-internal-sql-addr}/materialize
+ALTER SYSTEM SET enable_auto_scaling_strategy = true
+ALTER SYSTEM SET enable_hydration_burst = true
+
+> CREATE CLUSTER cc_cutover_burst (SIZE 'scale=1,workers=1', REPLICATION FACTOR 1, AUTO SCALING STRATEGY = (ON HYDRATION (HYDRATION SIZE = 'scale=2,workers=2', LINGER DURATION = '600s')))
+
+> CREATE TABLE cc_cutover_burst_t (id int)
+> INSERT INTO cc_cutover_burst_t VALUES (1)
+
+> CREATE MATERIALIZED VIEW cc_cutover_burst_slow IN CLUSTER cc_cutover_burst AS
+ SELECT mz_unsafe.mz_sleep(id * 3600) AS s FROM cc_cutover_burst_t
+
+> SELECT r.size, count(*) FROM mz_cluster_replicas r JOIN mz_clusters c ON r.cluster_id = c.id WHERE c.name = 'cc_cutover_burst' GROUP BY r.size ORDER BY r.size
+scale=1,workers=1 1
+scale=2,workers=2 1
+
+$ set-from-sql var=cc_cutover_burst_replica_id
+SELECT r.id::text
+FROM mz_cluster_replicas r
+JOIN mz_clusters c ON c.id = r.cluster_id
+WHERE c.name = 'cc_cutover_burst' AND r.size = 'scale=2,workers=2'
+
+# Control arm: the record path reshapes around the burst, whose replica keeps
+# running beside the new overlap replica.
+> ALTER CLUSTER cc_cutover_burst SET (SIZE 'scale=1,workers=2')
+
+> SELECT r.size, r.id::text = '${cc_cutover_burst_replica_id}' FROM mz_cluster_replicas r JOIN mz_clusters c ON r.cluster_id = c.id WHERE c.name = 'cc_cutover_burst' ORDER BY r.size
+scale=1,workers=1 false
+scale=1,workers=2 false
+scale=2,workers=2 true
+
+# The reshape is wedged (the target can never hydrate), so the operator forces
+# it with the escape hatch, re-stating the in-flight target exactly as the
+# cc_cutover section above does. The overlap replica is kept because it already
+# has the target shape, the baseline is retired, and the burst replica is
+# neither, so it must be left exactly as it is: same id, still running.
+> ALTER CLUSTER cc_cutover_burst SET (SIZE 'scale=1,workers=2') WITH (WAIT FOR '0s')
+
+> SELECT r.size, r.id::text = '${cc_cutover_burst_replica_id}' FROM mz_cluster_replicas r JOIN mz_clusters c ON r.cluster_id = c.id WHERE c.name = 'cc_cutover_burst' ORDER BY r.size
+scale=1,workers=2 false
+scale=2,workers=2 true
+
+# Liveness synchronizer: a reconfiguration elsewhere proves the controller ran
+# many ticks since the cut-over, so the count below is a settled state and not
+# one the controller has yet to churn.
+> CREATE CLUSTER cc_cutover_burst_tick (SIZE 'scale=1,workers=1', REPLICATION FACTOR 1)
+> ALTER CLUSTER cc_cutover_burst_tick SET (REPLICATION FACTOR 2)
+> SELECT count(*) FROM mz_cluster_replicas r JOIN mz_clusters c ON r.cluster_id = c.id WHERE c.name = 'cc_cutover_burst_tick'
+2
+> DROP CLUSTER cc_cutover_burst_tick
+
+# One burst replica was created across both reshapes, so neither bounced it.
+> SELECT count(*) FROM mz_catalog.mz_audit_events WHERE object_type = 'cluster-replica' AND details->>'cluster_name' = 'cc_cutover_burst' AND details->>'reason' = 'hydration-burst'
+1
+
+> DROP CLUSTER cc_cutover_burst CASCADE
+> DROP TABLE cc_cutover_burst_t
+
$ postgres-execute connection=postgres://mz_system@${testdrive.materialize-internal-sql-addr}/materialize
ALTER SYSTEM SET unsafe_enable_unstable_dependencies = falsecurrently fails:
cluster-controller.td:1504:1: error: non-matching rows: expected:
[["scale=1,workers=2", "false"], ["scale=2,workers=2", "true"]]
got:
[["scale=1,workers=2", "false"], ["scale=2,workers=2", "false"]]
Poor diff:
+ scale=2,workers=2 false
- scale=2,workers=2 true
|
Confirmed, and thanks for the test. It reproduces exactly, and the id flip in your output is the tell: the burst replica is dropped and recreated rather than kept. The mechanism is that the cut-over hand-builds its desired set as It is a regression from this PR specifically. Before it, a zero-timeout commit wrote a record and let the controller converge, and a tick unions all four strategies, so the burst kept its replica. It only becomes reachable once #38104 removes the @mtabebe spotted the same thing further up this PR. Your test is better than what I had sketched, in particular the audit-event count, which catches the bounce even if the id assertion somehow passes. Taking it as the regression test. On handling it gracefully, agreed that is the bar. The repair is to stop hand-building the contribution and run the real strategy set, since |
86f1766 to
fbc61b7
Compare
|
On the signal dependency: it may dissolve if the cut-over runs only phase 2. In any post-write state the cut-over can produce, Phase 1 the cut-over has no need to run: the writes it would produce (burst arm/teardown, on-refresh normalization) land on the following ticks, same as after a plain config write. And since the burst shape is the record's One wrinkle: "phase 2 reads no signals in these states" is a per-strategy fact, not a trait contract, and |
fbc61b7 to
958d547
Compare
|
Both findings were valid and are fixed in the rewritten Part 3. Scoped configuration is no longer controller-specific. The coordinator's catalog transaction machinery now derives contexts from concrete First-tick resource validation now starts from live inventory and applies the target creates and realized retires as a signed delta. It reuses materialized target replicas and carries unowned replicas. A warranted durable burst reserves count and credits even if its replica has not materialized yet. If AZ, logging, or compression changes, the model accounts for replacing the old burst instead of adding a second one. The tight regression now changes both size and replication factor, so it reaches the reshape path rather than the planner's RF-only The exact adapter clippy command is clean. Fresh CI is running on |
52a5f99 to
18b429a
Compare
|
Force-pushed a rebase onto current main, plus a design change we settled on The previous revision tried to predict a reshape's transient resource footprint Two consequences worth reviewing closely:
Two checks intentionally remain, both exact rather than predictive: a target's Foreground mode now reports a specific insufficient-resources error instead of |
QA LLM Review1. HIGH -- Resource-exhausted bursts enter a self-waking catalog write loop
Clearing the burst record on DetailsFor example, a valid RF-1 cluster at Keep a durable resource-exhausted or backoff state that |
| - `burst: Option<BurstState>` — the analogous record for an active hydration burst: the `burst_size` of the in-flight burst replica, a `linger_duration`, and the timestamp at which we observed the steady-state replicas as hydrated. Burst is controller-initiated (not tied to an `ALTER`), the strategy writes the record when we determine burst is needed. It is cleared when the burst tears down. | ||
|
|
||
| An `ALTER CLUSTER SET (...)` that changes a replica's **config shape** writes the `reconfiguration` record with `status = InProgress` in a transaction and returns; the realized config is left untouched until the controller cuts over. Shape changes are `SIZE`, logging (`INTROSPECTION ...`), and `AVAILABILITY ZONES`. When no reconfiguration is active, changes that need no overlap (replication-factor-only, etc.) skip the record and update the realized config directly. But once an in-progress `reconfiguration` record is present, every further `ALTER` instead **folds into it**, overwriting its `target`, deadline, and status. So the realized config is advanced only by the controller at cut-over, and no direct config write ever races an in-flight transition. Re-targeting to a new non-realized shape writes `status = InProgress`. ALTER-back to the realized shape writes `status = Cancelled`, which immediately disengages the strategy and lets the target replicas fall out of the desired set. The controller's job is to converge the actual replica set onto the active target and, at cut-over, advance the realized config to match. | ||
| An `ALTER CLUSTER SET (...)` that changes a replica's **config shape** writes the `reconfiguration` record with `status = InProgress` in a transaction and returns. The realized config is left untouched until the controller cuts over. Shape changes are `SIZE`, logging (`INTROSPECTION ...`), and `AVAILABILITY ZONES`. A replication-factor-only change on an otherwise steady `MANUAL` cluster uses the same record. Its target has the realized replica shape and the requested factor, so existing replicas satisfy target slots and the controller creates or drops only the count difference at cut-over. The planner continues to reject `WAIT` on an RF-only statement because the SQL `WAIT` surface remains limited to replica-shape changes. Once an in-progress `reconfiguration` record is present, a further shape `ALTER` **folds into it**, overwriting its `target`, deadline, and status. Replication-factor changes remain refused while the record is active because cut-over writes the target factor atomically. Re-targeting to the realized configuration writes `status = Cancelled`, which immediately disengages the strategy and lets target-only replicas fall out of the desired set. The controller's job is to converge the actual replica set onto the active target and, at cut-over, advance the realized config to match. |
There was a problem hiding this comment.
doesn't this " Replication-factor changes remain refused while the record is active because cut-over writes the target factor atomically." contradict what we say just above, that replication factor changes fold into an in-flight record. Make sure this whole paragraph is correct and matches our intent and is concise
|
Good catch on the burst loop, that one is real and I took the fix further than Clearing the burst record on Separately, the same review pass turned up a worse problem in the opposite Both are in I did not act on a generic typed outcome for non-resource create failures. |
| /// | ||
| /// [`ClusterController::shed_decision`]: crate::ClusterController::shed_decision | ||
| fn forced_cutover_pending(state: &ClusterState, now: Timestamp) -> bool { | ||
| state.reconfiguration.as_ref().is_some_and(|record| { |
There was a problem hiding this comment.
are we sure we always retire the reconfiguration record on time, so that we don't accidentally "suppress" the "new" replicas on a forced cutover, right after?
b71b025 to
057f79b
Compare
|
Yes, and it holds for a structural reason rather than by timing. The baseline yields on Two failure modes worth naming, since "on time" is the right thing to worry
Backed by three tests rather than by argument. |
| /// the first tick regardless.) | ||
| /// | ||
| /// The one case where the baseline steps aside is a forced cut-over, see | ||
| /// [`forced_cutover_pending`]. |
There was a problem hiding this comment.
| /// let the unchanged policy arm the same burst on the next tick. Each | ||
| /// cycle would write a start and a finish, allocate a replica id, and wake | ||
| /// the next reconciliation immediately. An unaffordable burst keeps | ||
| /// retrying its create instead, which costs one rejected transaction per |
There was a problem hiding this comment.
One correction on the retrying-burst behavior: it is not free of catalog writes. apply_cluster_decisions allocates replica ids durably before every apply (one commit per CreateReplica), and a rejected apply never reclaims them, so an unaffordable burst costs one durable commit and one burned id per tick. Validating the limits before allocating, or holding pre-allocated ids across retries in memory, would make the retry as cheap as described without a format change.
| // `catalog_transact`'s replica accounting, because the controller | ||
| // materializes the replicas on a later tick rather than this transaction | ||
| // emitting creates. Without the check the ALTER would succeed and the | ||
| // controller would then fail its own create transaction on every tick. |
There was a problem hiding this comment.
Pre-existing, but the retry-per-tick behavior now accepted for bursts makes it worth naming: this check counts by config, so an RF raise that fits by config exceeds the limit once the warranted burst joins the union. The ALTER returns OK, nothing is surfaced, and the top-up retries every tick until the burst lingers out (indefinitely if the steady set cannot hydrate). Counting the live burst here would close it.
b0fc13c to
efaffbf
Compare
|
Rebuilt and restacked this on current main. The controller-path fixes discussed in the recent review threads are now #38580. The GitHub stack is now #38580 -> #38103 -> #38104 -> #38446 (reference only). |
efaffbf to
d7ff178
Compare
d7ff178 to
3a1be64
Compare
fe8f383 to
3fc3358
Compare
3798c93 to
a850316
Compare
The controller owns every managed cluster's replica set. Shape-changing ALTERs return through the durable reconfiguration-record path before reaching the managed-to-managed config write. The `controller_owns = true` guard therefore left the staged producer and sequencer reconciliation branches unreachable. Deleting those branches does not change managed-cluster behavior. Delete the WaitForHydrated and Finalize stages, pending overlap replicas, per-connection pending ALTER state, sequencer replica create/drop branches, and their dead op and error plumbing. Keep the durable pending field and catalog-open cleanup so a replica stranded by an older binary is still reaped after upgrade. The surviving managed-to-managed path is a config-only write. The cluster controller reconciles replication-factor changes and scheduled-cluster shape changes on its next tick.
Record the narrow compatibility reason the durable pending field and catalog-open cleanup remain even though the controller does not use pending replicas as active lifecycle state.
a850316 to
bc37a9e
Compare
4 participants
Motivation
The staged cluster-reconfiguration machine is unreachable. Managed cluster
shape changes return through the durable reconfiguration-record path, and
sequence_alter_cluster_managed_to_managedhascontroller_owns = true, so itsstaged/direct branches have no live producer.
Controller-path correctness fixes are split into #38580, the preceding PR. This PR is
dead-code removal.
Description
Deletes:
ClusterStage::WaitForHydratedandClusterStage::FinalizeNeedsFinalizationandPENDING_REPLICA_SUFFIXOp::UpdateClusterReplicaConfigand error plumbingThe surviving managed-to-managed path is a config-only write. The controller
reconciles replication-factor changes and scheduled-cluster shape changes on its
next tick.
The durable
pendingfield and catalog-open cleanup stay. An older binary mayhave crashed after creating a pending replica, and an upgraded binary must still
reap that stranded state.
Net diff: +100/-806. Most additions are the small config-only
replacement for the deleted reconciliation function and comments documenting
upgrade cleanup.
Verification
cargo clippy -p mz-adapter -p mz-cluster-controller --all-targets -- -D warningscargo check -p mz-adapter -p mz-cluster-controllercargo fmt --all -- --checkthe production path, not the deleted implementation
Integration suites are left to PR CI.