Skip to content

Commit 64f5011

Browse files
committed
compute: move the replica's config set into the replica task
The replica task is where per-replica command specialization already lives, and its configuration is not the hydration interceptor's private business. Give `ReplicaTask` a `replica_dyncfg` holding what its replica reads, kept current from the configuration commands passing through, and hand it to `SequentialHydration` on each call. The interceptor now holds no configuration of its own, so it cannot read the environment-wide value by accident. It reads the replica's or nothing. Prune per-replica dyncfg overrides when a replica is dropped. The coordinator re-pushes the override map only when the scoped configuration itself changes, so a dropped replica's entry was otherwise retained until the next such change, and an environment churning replicas accumulated them. Correct the justification on `compute_subscribe_snapshot_optimization`. The two reads need not agree. The replica-side read only ever puts a snapshot back, so a disagreement costs work rather than correctness. It is environment-scoped because the plan-time read has no replica in scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
1 parent db5feaf commit 64f5011

7 files changed

Lines changed: 195 additions & 86 deletions

File tree

src/compute-client/src/controller.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -795,6 +795,12 @@ impl ComputeController {
795795

796796
instance.replicas.remove(&replica_id);
797797

798+
// The coordinator only re-pushes the override map when the scoped
799+
// configuration itself changes, so a dropped replica's entry would
800+
// otherwise be retained until the next such change.
801+
self.replica_dyncfg_overrides.remove(&replica_id);
802+
803+
let instance = self.instance_mut(instance_id).expect("validated");
798804
instance.call(move |i| i.remove_replica(replica_id).expect("validated"));
799805

800806
Ok(())

src/compute-client/src/controller/instance.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,6 +1294,11 @@ impl Instance {
12941294
pub fn remove_replica(&mut self, id: ReplicaId) -> Result<(), ReplicaMissing> {
12951295
let replica = self.replicas.remove(&id).ok_or(ReplicaMissing(id))?;
12961296

1297+
// The coordinator only re-pushes the override map when the scoped configuration itself
1298+
// changes, so a dropped replica's entry would otherwise be retained until the next such
1299+
// change.
1300+
self.replica_dyncfg_overrides.remove(&id);
1301+
12971302
// Before dropping the replica state (and the contained input read holds), log read holds
12981303
// that are the last line of defense against compaction of a dataflow's storage inputs. If
12991304
// the corresponding global read hold has already been released, dropping the per-replica

src/compute-client/src/controller/replica.rs

Lines changed: 90 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use anyhow::bail;
1717
use mz_build_info::BuildInfo;
1818
use mz_cluster_client::client::ClusterReplicaLocation;
1919
use mz_compute_types::dyncfgs::ENABLE_COMPUTE_REPLICA_EXPIRATION;
20-
use mz_dyncfg::ConfigSet;
20+
use mz_dyncfg::{ConfigSet, ConfigUpdates};
2121
use mz_ore::channel::InstrumentedUnboundedSender;
2222
use mz_ore::retry::{Retry, RetryState};
2323
use mz_ore::task::AbortOnDropHandle;
@@ -95,6 +95,7 @@ impl ReplicaClient {
9595
epoch,
9696
metrics: metrics.clone(),
9797
connected: Arc::clone(&connected),
98+
replica_dyncfg: seed_replica_dyncfg(&dyncfg),
9899
dyncfg,
99100
}
100101
.run(),
@@ -131,6 +132,27 @@ impl ReplicaClient {
131132

132133
type ComputeCtpClient = transport::Client<ComputeCommand, ComputeResponse>;
133134

135+
/// Creates a replica's effective configuration, seeded from the environment-wide one.
136+
///
137+
/// The seed covers the window before the first configuration command arrives, and is replaced
138+
/// wholesale by the snapshot that `CreateInstance` carries.
139+
fn seed_replica_dyncfg(dyncfg: &ConfigSet) -> ConfigSet {
140+
let replica_dyncfg = mz_dyncfgs::all_dyncfgs();
141+
ConfigUpdates::from(dyncfg).apply(&replica_dyncfg);
142+
replica_dyncfg
143+
}
144+
145+
/// Applies the configuration a command carries, if any, to a replica's effective configuration.
146+
///
147+
/// `CreateInstance` carries a full snapshot, `UpdateConfiguration` the subsequent deltas.
148+
fn apply_config_command(command: &ComputeCommand, dyncfg: &ConfigSet) {
149+
match command {
150+
ComputeCommand::CreateInstance(config) => config.initial_config.apply(dyncfg),
151+
ComputeCommand::UpdateConfiguration(params) => params.dyncfg_updates.apply(dyncfg),
152+
_ => (),
153+
}
154+
}
155+
134156
/// Configuration for `replica_task`.
135157
struct ReplicaTask {
136158
/// The ID of the replica.
@@ -150,8 +172,21 @@ struct ReplicaTask {
150172
metrics: ReplicaMetrics,
151173
/// Flag to report successful replica connection.
152174
connected: Arc<AtomicBool>,
153-
/// Dynamic system configuration.
175+
/// The controller's environment-wide dynamic system configuration.
154176
dyncfg: Arc<ConfigSet>,
177+
/// This replica's effective dynamic system configuration.
178+
///
179+
/// Holds what the replica itself reads, including its scoped overrides, as opposed to
180+
/// [`Self::dyncfg`], which holds the environment-wide values. Seeded from the environment-wide
181+
/// configuration and then kept current from the configuration commands passing through this
182+
/// task, which `Instance::specialize_command_for_replica` has already specialized for this
183+
/// replica. Read it for any `ParameterScope::Replica` config the controller realizes on this
184+
/// replica's behalf, else the scope declaration is a silent no-op.
185+
///
186+
/// A set of its own, rather than a clone of the controller's: a cloned `ConfigSet` shares its
187+
/// values with the original, so applying this replica's overrides to a clone would overwrite
188+
/// the environment-wide configuration for everyone.
189+
replica_dyncfg: ConfigSet,
155190
}
156191

157192
impl ReplicaTask {
@@ -229,7 +264,7 @@ impl ReplicaTask {
229264
// The sequential hydration interceptor holds back `Schedule` commands and releases them as
230265
// hydration capacity frees up. It is recreated per incarnation, matching the lifetime of
231266
// the connection: any in-flight hydration state is reset when we reconnect.
232-
let mut hydration = SequentialHydration::new(&self.dyncfg, self.metrics.clone());
267+
let mut hydration = SequentialHydration::new(self.metrics.clone());
233268

234269
loop {
235270
select! {
@@ -242,7 +277,8 @@ impl ReplicaTask {
242277

243278
self.specialize_command(&mut command);
244279
self.observe_command(&command);
245-
for command in hydration.absorb_command(command) {
280+
apply_config_command(&command, &self.replica_dyncfg);
281+
for command in hydration.absorb_command(command, &self.replica_dyncfg) {
246282
client.send(command).await?;
247283
}
248284
},
@@ -254,7 +290,7 @@ impl ReplicaTask {
254290

255291
self.observe_response(&response);
256292

257-
for command in hydration.observe_response(&response) {
293+
for command in hydration.observe_response(&response, &self.replica_dyncfg) {
258294
client.send(command).await?;
259295
}
260296

@@ -320,3 +356,52 @@ impl ReplicaTask {
320356
);
321357
}
322358
}
359+
360+
#[cfg(test)]
361+
mod tests {
362+
use mz_compute_types::dyncfgs::HYDRATION_CONCURRENCY;
363+
364+
use crate::protocol::command::{ComputeParameters, InstanceConfig};
365+
366+
use super::*;
367+
368+
/// A replica's effective configuration tracks the configuration commands passing through its
369+
/// task, which carry the replica's scoped overrides, and leaves the environment-wide
370+
/// configuration alone.
371+
#[mz_ore::test]
372+
fn replica_dyncfg_tracks_config_commands() {
373+
let env_wide = mz_dyncfgs::all_dyncfgs();
374+
let mut updates = ConfigUpdates::default();
375+
updates.add(&HYDRATION_CONCURRENCY, 1);
376+
updates.apply(&env_wide);
377+
378+
let replica_dyncfg = seed_replica_dyncfg(&env_wide);
379+
assert_eq!(HYDRATION_CONCURRENCY.get(&replica_dyncfg), 1);
380+
381+
// A replica-scoped override arrives merged into the create-time snapshot.
382+
let mut initial_config = ConfigUpdates::default();
383+
initial_config.add(&HYDRATION_CONCURRENCY, 2);
384+
let create = ComputeCommand::CreateInstance(Box::new(InstanceConfig {
385+
logging: Default::default(),
386+
expiration_offset: None,
387+
peek_stash_persist_location: mz_persist_client::PersistLocation::new_in_mem(),
388+
arrangement_dictionary_compression: false,
389+
initial_config,
390+
}));
391+
apply_config_command(&create, &replica_dyncfg);
392+
assert_eq!(HYDRATION_CONCURRENCY.get(&replica_dyncfg), 2);
393+
394+
// And into subsequent configuration updates.
395+
let mut dyncfg_updates = ConfigUpdates::default();
396+
dyncfg_updates.add(&HYDRATION_CONCURRENCY, 3);
397+
let update = ComputeCommand::UpdateConfiguration(Box::new(ComputeParameters {
398+
dyncfg_updates,
399+
..Default::default()
400+
}));
401+
apply_config_command(&update, &replica_dyncfg);
402+
assert_eq!(HYDRATION_CONCURRENCY.get(&replica_dyncfg), 3);
403+
404+
// The environment-wide configuration is untouched by the replica's overrides.
405+
assert_eq!(HYDRATION_CONCURRENCY.get(&env_wide), 1);
406+
}
407+
}

0 commit comments

Comments
 (0)