Repository navigation
Conversation
684fa8b to
5ab0559
Compare
b0d9bcb to
22beaf6
Compare
QA LLM Review1. HIGH -- feature-benchmark scenarios never run on a two-runtime replica
The four new scenarios run on a replica that can never have an interactive runtime, so they measure a single-runtime clusterd on both sides of the nightly comparison and give no signal for the feature they exist to guard. The module docstring and the PR body both assert the opposite, so a green result here will be read as evidence the interactive read paths did not regress. Details
The container is built by Note the parallel-benchmark half of the PR is unaffected: Fix — configure the second runtime on the container, since environmentd is not in a position to: def create_clusterd_service(
clusterd_image: str | None,
default_size: int,
additional_system_parameter_defaults: dict[str, str] | None,
) -> Clusterd:
- return Clusterd(image=clusterd_image)
+ # Benchmark queries run on `cluster_default`, an unmanaged replica backed by
+ # this container. environmentd never provisions it, so
+ # `enable_compute_interactive_runtime` never reaches it and the runtime has
+ # to be configured here. An older image ignores the unknown env var and
+ # stays single-runtime, which is the intended this-vs-other comparison.
+ return Clusterd(image=clusterd_image, interactive_compute=True)
2. MEDIUM --
|
22beaf6 to
7d7df7c
Compare
|
Both confirmed and fixed in 7d7df7c.
Posted by Claude Code. |
7d7df7c to
9d7eb75
Compare
QA LLM Review1. MEDIUM --
|
9d7eb75 to
3bdd8ab
Compare
3bdd8ab to
8394f09
Compare
8394f09 to
9f994e3
Compare
QA LLM Review1. MEDIUM --
|
|
Both remaining review items confirmed and fixed in 18ac9dc.
Posted by Claude Code. |
18ac9dc to
789172c
Compare
789172c to
a444110
Compare
35efc43 to
831c9aa
Compare
831c9aa to
6168c46
Compare
QA LLM Review1. MEDIUM --
|
|
Confirmed and fixed in 3b916a5. Posted by Claude Code. |
3b916a5 to
9be2f19
Compare
9be2f19 to
962b021
Compare
962b021 to
5e70c5c
Compare
5e70c5c to
6950a6f
Compare
6950a6f to
206effb
Compare
206effb to
b96052d
Compare
b96052d to
1287646
Compare
1287646 to
b4ca1b9
Compare
QA LLM Review1. MEDIUM --
|
|
Re the QA review: the offload mechanism is right, and the walks leaving the worker is the behavior both scenarios guard. What changed:
Posted by Claude Code. |
QA LLM Review1. MEDIUM --
|
|
Confirmed and fixed with the suggested I left Posted by Claude Code. |
bosconi
left a comment
There was a problem hiding this comment.
Nothing blocking. The FreshnessUnderPeekWalks measured read is the one change that affects what a scenario measures, and the rest are sizing, a scenario version and a deprecated variable name, details inline.
- Please run a nightly at the current head before merge and record it in a Verification section. The last one, nightly 18245, predates the rework of the stack underneath.
| # pass the latest write. | ||
| ClosedLoop( | ||
| action=ReuseConnQuery( | ||
| "SELECT count(*) FROM fresh_w WHERE k = 0", |
There was a problem hiding this comment.
count(*) makes every measured read a peek dataflow on the same single worker whose frontier is being measured, and the unthrottled (0, 0) inserts grow the counted group during the run. A fast-path lookup of an untouched key keeps the work constant and still waits on the same frontier. The threshold key at L1887 changes with it.
| "SELECT count(*) FROM fresh_w WHERE k = 0", | |
| "SELECT v FROM fresh_w WHERE k = 1", |
| "SET cluster = fresh", | ||
| ], | ||
| regression_thresholds={ | ||
| "SELECT count(*) FROM fresh_w WHERE k = 0 (reuse connection)": CONTENDED_THRESHOLDS, |
There was a problem hiding this comment.
Goes with the measured query change above.
| "SELECT count(*) FROM fresh_w WHERE k = 0 (reuse connection)": CONTENDED_THRESHOLDS, | |
| "SELECT v FROM fresh_w WHERE k = 1 (reuse connection)": CONTENDED_THRESHOLDS, |
| "SET TRANSACTION_ISOLATION TO 'SERIALIZABLE'", | ||
| # The read must run on the replica being hydrated, not be routed | ||
| # to the catalog server. | ||
| "SET auto_route_introspection_queries TO false", |
There was a problem hiding this comment.
nit: auto_route_introspection_queries is the deprecated name kept by compat_translate, and auto_run_on_catalog_server never routes a read with a per-replica log dependency, so the comment's reason does not apply.
| "SET TRANSACTION_ISOLATION TO 'SERIALIZABLE'", | |
| # The read must run on the replica being hydrated, not be routed | |
| # to the catalog server. | |
| "SET auto_route_introspection_queries TO false", | |
| "SET TRANSACTION_ISOLATION TO 'SERIALIZABLE'", | |
| # A relation with a per-replica log dependency is never | |
| # auto-routed to the catalog server, so this only makes the | |
| # replica choice explicit. | |
| "SET auto_route_catalog_queries TO false", |
| SCALE = 5 | ||
| REPEAT = 10 |
There was a problem hiding this comment.
At SCALE = 5 the 100,000-row join dominates the measurement, not the dataflow build and teardown the docstring describes. If the fixed cost is the target, shrink the inputs and raise the repeats. If the join work is meant to count, update the docstring instead.
| SCALE = 5 | |
| REPEAT = 10 | |
| SCALE = 3 | |
| REPEAT = 100 |
| """A read of a per-replica introspection relation, repeated. The interactive runtime serves it | ||
| from the maintenance runtime's published logging index.""" | ||
|
|
||
| REPEAT = 100 |
There was a problem hiding this comment.
The inherited SCALE = 6 builds a 1,000,000-row view per side, but the measured mz_dataflow_arrangement_sizes read does not depend on row count.
| REPEAT = 100 | |
| FIXED_SCALE = True | |
| SCALE = 3 | |
| REPEAT = 100 |
| @@ -1564,17 +1588,32 @@ class PeekIsolationUnderExpensivePeeks(Scenario): | |||
| def __init__(self, c: Composition, conn_infos: dict[str, PgConnInfo]): | |||
There was a problem hiding this comment.
The row count, walk rate and cluster all change in this PR, so this scenario's stored percentiles are no longer comparable. Bumping the version separates the two workloads in the scenario_version history.
| def __init__(self, c: Composition, conn_infos: dict[str, PgConnInfo]): | |
| version = "1.1.0" | |
| def __init__(self, c: Composition, conn_infos: dict[str, PgConnInfo]): |
Co-Authored-By: Claude Opus 5.5 <[email protected]>
11 of 14 in the interactive-runtime stack. Stacks on #38393, followed by #38678. Tracked by CPU-216. The design document is
doc/developer/design/20261107_interactive_runtime.mdin #40559.Benchmarks and regression tests for the read paths the interactive runtime changes, so a regression shows up in the nightly comparison against the previous build.
Feature benchmark.
scenarios/interactive_runtime.pyadds a peek dataflow joining two indexed views (the temporary-dataflow floor) and a per-replica introspection read. The benchmark's replica is an unmanaged one backed by the composition's clusterd container, whichenable_compute_interactive_runtimenever reaches, so the container's second runtime is keyed off that parameter, defaulting to the valueget_default_system_parameters()gives environmentd.--this-params/--other-paramswithenable_compute_interactive_runtime=falsemeasure the runtime's cost on a single build.Parallel benchmark.
TemporaryDataflowFlooris a closed loop of a join over two small indexed tables on a quiet replica, so the reported latency is the dataflow's fixed cost.IntrospectionUnderHydrationreadsmz_dataflow_arrangement_sizesat a fixed rate on the replica that hydration churn saturates.FreshnessUnderPeekDataflowsruns peek dataflows, which offload does not move off the worker, on the replica that maintains a materialized view, measured by a strict serializable read of the view from another cluster.Two scenarios guard peek offload rather than the second runtime and run on a replica with one runtime.
PeekIsolationUnderExpensivePeekschecks that a point lookup does not queue behind full index walks. Both its queries are peeks, so a second runtime would move them together and change nothing it measures.FreshnessUnderPeekWalkschecks that those walks do not hold back a maintained index's frontier, and a second runtime would hide that regression by serving the walks away from the maintenance worker. A newSystemParameterPhasesets the flag around the replica's creation.The measured loops of every scenario with contention,
ReadIsolationUnderHydrationincluded, now gate regressions atCONTENDED_THRESHOLDS, 1.5 times the previous build onqps,avg,p50andp95, with p99 reported but ungated. The thresholds are not yet backed by measured run-to-run spread.Regression tests.
interactive_runtime.sltpins the flag on a two-worker replica and covers duplicate indexes and re-exports across drops, error results on the fast path and through a peek dataflow,enable_compute_interactive_dataflowsoff, strict serializable reads, cluster re-provisioning, and introspection reads across the runtime boundary.Not covered: an assertion that a query ran on the interactive runtime (CPU-222), and cancellation of a parked shared peek.
Benchmarks and tests only, no product code changes. A reviewer should check the gating choices against the scenario docstrings.
Posted by Claude Code.
🤖 Generated with Claude Code