Repository navigation
Conversation
91defbe to
5eecaa4
Compare
5eecaa4 to
e6d27f4
Compare
a985417 to
3cc5402
Compare
3cc5402 to
15cc33c
Compare
9311614 to
eb80f6a
Compare
eb80f6a to
3767df8
Compare
3767df8 to
9fcfab0
Compare
9fcfab0 to
c929c6d
Compare
c929c6d to
8f441a1
Compare
8f441a1 to
67eb09f
Compare
67eb09f to
dda7a03
Compare
25522fe to
3b351a1
Compare
3b351a1 to
6a2c60c
Compare
6a2c60c to
cb55394
Compare
cb55394 to
dca62ce
Compare
dca62ce to
9f14eeb
Compare
9f14eeb to
8d03245
Compare
8d03245 to
2ed3a19
Compare
2ed3a19 to
cfc275a
Compare
cfc275a to
4fb99ee
Compare
bosconi
left a comment
There was a problem hiding this comment.
The two-runtime plumbing looks sound, but the interactive config derivation lost its tests and the resource accounting needs stating, details inline.
- Nothing in CI runs the two-runtime clusterd path until #38393 turns the flag on. Please add a Verification section to the description saying so, and land the stack together.
| /// | ||
| /// `interactive_runtime` says whether to launch a second, interactive compute timely runtime | ||
| /// alongside the primary one. The caller resolves it per replica, see |
There was a problem hiding this comment.
Read the flag with ENABLE_COMPUTE_INTERACTIVE_RUNTIME.get_with_overrides(&self.dyncfg, overrides), the way ENABLE_UNIFIED_CLUSTER is read a few lines below (L719-L736). That drops the interactive_runtime parameter from create_replica, provision_replica and both adapter call sites, and removes CatalogState::replica_scoped with its string reparse.
| /// | ||
| /// Parses through `ConfigType`, because a stored override is a var-format string. `bool` | ||
| /// formats as `on`/`off`, which the value type's own `str::parse::<bool>()` rejects. | ||
| pub fn replica_scoped<D: ConfigDefault>( |
There was a problem hiding this comment.
If the flag moves to get_with_overrides in provision_replica, this helper can go. If it stays, it needs tests over Catalog::with_debug: a stored "on" override returns true, and an unparsable override falls back to the environment value, not config.default().
| let interactive_compute_timely_config = args.interactive_compute_timely_config.map( | ||
| |mut interactive| { | ||
| interactive.process = args.process; | ||
| assert_eq!( | ||
| ( | ||
| compute_timely_config.workers, | ||
| compute_timely_config.addresses.len() | ||
| ), | ||
| (interactive.workers, interactive.addresses.len()), | ||
| "interactive and maintenance compute runtimes must run equally many workers per \ | ||
| process over equally many processes", | ||
| ); | ||
| interactive | ||
| }, | ||
| ); |
There was a problem hiding this comment.
This derivation lost the prepare_interactive_compute_config unit tests from dca62ce. Dropping interactive.process = args.process or loosening the assert would now pass CI, and every process but ordinal 0 would join the interactive cluster as process 0. Please pull it back out as a free function next to multiplex_compute and restore the tests, including an equal-product, different-shape case (workers = 4 over 1 address vs workers = 2 over 2).
| // `peer_addresses` panics on a port name absent from `ports` above, so the | ||
| // port and this argument are added under the same condition. | ||
| let interactive_compute_timely_config = TimelyConfig { | ||
| workers: location.allocation.workers.get(), |
There was a problem hiding this comment.
The interactive runtime doubles the Timely worker threads under unchanged cpu_limit, memory_limit and zero_copy_limit, and with enable_worker_core_affinity both runtimes pin worker i to the same core. Please state the intended resource accounting in the description: should the interactive runtime get fewer workers, or should the allocation grow when the flag is on?
| let interactive_compute_timely_config = TimelyConfig { | ||
| workers: location.allocation.workers.get(), | ||
| addresses: assigned.peer_addresses(INTERACTIVE_PORT_NAME), | ||
| ..compute_proto_timely_config.clone() |
There was a problem hiding this comment.
nit: only Copy fields remain after the addresses override, so the base is never moved and the clone can go, as in the compute_timely_config construction above.
| ..compute_proto_timely_config.clone() | |
| ..compute_proto_timely_config |
| // The new flag parses into an `Option<TimelyConfig>`. | ||
| let interactive = timely_config(1, &["127.0.0.1:2103"]).to_string(); | ||
| let args = base(vec!["--interactive-compute-timely-config", &interactive]); | ||
| assert!(args.interactive_compute_timely_config.is_some()); |
There was a problem hiding this comment.
is_some() holds for any config. Comparing the value also covers the serde round trip the flag depends on:
| assert!(args.interactive_compute_timely_config.is_some()); | |
| assert_eq!( | |
| args.interactive_compute_timely_config, | |
| Some(timely_config(1, &["127.0.0.1:2103"])) | |
| ); |
With `--interactive-compute-timely-config`, clusterd runs two compute runtimes in one process, `Maintenance` and `Interactive`. Worker `i` of each takes the same per-worker sharing registry, so the two runtimes must run equally many workers per process over equally many processes, which clusterd asserts. A `Multiplexer` fronts both runtimes on the one controller endpoint. Without the flag the process runs a single `Solo` runtime, unchanged. `Placement` names the dataflow classes a runtime renders. A dataflow the runtime does not render records its exports as peers, and on the interactive runtime its index exports get a peer bundle in `traces`, which serves index peeks. Peers never enter `collections`, so only the runtime that renders a collection reports its frontiers. `ProcessGlobals` applies the process-wide settings from one runtime only. Both runtimes reconcile each connection on their own, and nothing orders the two reconciliations yet. Until the next change adds the reconcile barrier, two runtimes reconciling the same connection can bind a new incarnation's peer to the slot of the old incarnation of an index recreated at the same `as_of`, and then read that incarnation's frozen view. `enable_compute_interactive_runtime` is off, so no replica runs two runtimes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
8 of 14 in the interactive-runtime stack. Stacks on #38390, followed by #40558. Tracked by CPU-216. The design document is
doc/developer/design/20261107_interactive_runtime.mdin #40559.With
--interactive-compute-timely-config, clusterd runs two compute runtimes in one process: the first takesMaintenance, the secondInteractive, and workeriof each takes the same per-worker sharing registry. clusterd asserts that both run the same worker layout. AMultiplexerfronts both on the one controller endpoint. Without the flag the process runs a singleSoloruntime, unchanged.Placementnames the dataflow classes a runtime renders, andhandle_create_dataflowchecks it first. A dataflow the runtime does not render only records its exports as peers, with a peer bundle intracesfor each index on the interactive runtime. Peers never entercollections, so only the runtime that renders a collection reports its frontiers.ScheduleandAllowWritesare no-ops on a peer,AllowCompactionmoves its hold, and the empty frontier removes it. Logging indexes are peers too, at the minimum time.compute_state.traceslike on any runtime. A peek whose index has not sealed its timestamp waits inqueued_peeks. A peek naming an index neither hosted nor held as a peer is answered with an error.ProcessGlobalsapplies the process-wide settings from the maintenance runtime only.process_globals.rsdocuments the window on a process's first connection in which the interactive runtime runs with the previous values.Not in this PR: the reconcile barrier, which #40558 adds. Until then, two runtimes reconciling a connection that recreates an index at the same
as_ofcan bind the new peer to the old incarnation's slot. The flag being off keeps that unreachable.enable_compute_interactive_runtimeis replica-scoped and off. It is read when a replica is provisioned, so it is not a live toggle, and is resolved inenvironmentd, which decides the replica's ports. The flag is inUNINTERESTING_SYSTEM_PARAMETERSand pinned toFALSEin parallel-workload'sFlipFlagsActionuntil #38393 turns it on in tests.enable_compute_interactive_dataflows, on by default and flipped byFlipFlagsAction, keeps peek dataflows on the maintenance runtime when off.Tests in
compute_state/tests.rscover peeks over a peer bundle, peeks waiting for publication and for the seal, and a build over an unpublished peer index.A reviewer should check the peer paths in
handle_create_dataflowandhandle_allow_compaction, and the split ofProcessGlobals.Posted by Claude Code.
🤖 Generated with Claude Code