Repository navigation
SS-500 - Lower storage persist sink lookahead to 2s in CI - #39602
Conversation
martykulma
changed the title
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
The change is small and correct. The main open question is whether the ancestor override is still needed now that v26.45 is released (see inline). Also, the description says "we expect this to be in production for everyone" while the code comment says it "defaults off in production", so it would help to pick one framing. Minor typo in the description: "the the".
Posted by Claude Code.
| # flag defaults off in production. | ||
| min_ancestor_mz_version_per_commit[ | ||
| "80e24400e7636cad60f7410abe0249bc87b25938" | ||
| ] = MzVersion.parse_mz("v26.45.0") |
There was a problem hiding this comment.
This override looks like a no-op on main. _get_override_commit_instead_of_version only fires when the comparison version is below v26.45.0. On main the cargo version is v26.46.0-dev and the previous published version is v26.45.0 (marked released in #39665), which contains 80e2440, so neither the merge-base nor the previous-release path triggers it. The only case where it fires is a v26.45.x release build compared against v26.44, and there it returns None, which skips the comparison rather than picking a different baseline.
The 5s regression came from comparing against v26.44 before v26.45 was released, so it should resolve without this entry. Could you either drop it, or note in the PR that it targets v26.45 release-branch benchmarks?
| config["hydration_history_collection_interval"] = "60s" | ||
|
|
||
| if version >= MzVersion.parse_mz("v26.41.0-dev"): | ||
| if version >= MzVersion.parse_mz("v26.45.0-dev"): |
There was a problem hiding this comment.
The gate move from v26.41.0-dev to v26.45.0-dev is correct but not mentioned in the PR description. #38935 added the dyncfg and this line in the same commit, and it first shipped in v26.45.0 (v26.44.0 does not contain it), so mzcompose was setting a parameter that v26.41 to v26.44 images don't know, which matters for upgrade and platform-check runs.
It also means the v26.45.0 baseline now runs with the same 2s as HEAD, so benchmarks compare like for like. That is the actual fix for the benchmark, and another reason the ancestor override looks redundant.
| # updates while it defaults off in production. One timestamp interval | ||
| # past the floor keeps the ceiling ahead of the data. A Postgres | ||
| # source's snapshot and first CDC rows become visible about this much | ||
| # later, which the PgCdc feature benchmarks measure. | ||
| config["storage_persist_sink_description_lookahead"] = "2s" |
There was a problem hiding this comment.
nit: "One timestamp interval past the floor" doesn't match the config. CI's default_timestamp_interval is 1s and gets randomized over ["100ms", "1s"], so 2s is two intervals, or twenty at 100ms. Maybe "at least one default_timestamp_interval (1s by default)".
nit: the PgCdc visibility-delay sentence is repeated in the override comment in version_ancestor_overrides.py. I'd keep it in one place.
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
Bot found some nits, but otherwise LGTM! Thank you
|
Thanks for the review! |
martykulma
changed the title
…#40206) ### Motivation Today a MySQL source snapshots all of its tables before it reads any of the binlog. This PR lets the replication operator read the binlog while the snapshot runs, as #39419 did for PostgreSQL. #40207 then lets each export's frontier advance on its own. NOTE: this needs an exception added for the 2s increase in snapshot performance, but I can't add it until I have a merge hash from main. Will do in a follow up PR. Closes: [SS-488](https://linear.app/materializeinc/issue/SS-488) ### Description All new behavior is keyed off `storage_persist_sink_description_lookahead` being nonzero. This is the same switch PostgreSQL uses. With a nonzero lookahead, the snapshot operator emits its rewind requests as soon as the snapshot GTID frontier is known. That happens before any table is read. The replication operator then reads the binlog while the tables are read. Every export still advances together. Replication data is staged in the dataflow until the snapshot completes. With the lookahead at zero, rewinds are emitted after the snapshot completes, as before. The snapshot operator reads the setting once at render time. That matches the value the persist sink reads in the same dataflow build. MySQL sources now count as OLTP sources in `render.rs`. A snapshotting MySQL export therefore gets the persist sink's snapshot ceiling and lookahead grouping, like a PostgreSQL export. The mzcompose default sets the lookahead to 2s. The MySQL initial load feature benchmarks should see their first rows about that much later, as the PgCdc benchmarks did after #39602. #39419 also added a leader check that the replication slot exists before the snapshot starts. MySQL needs no counterpart. The replication operator already checks `@@gtid_purged` against the resume upper before it waits for rewind requests. ### Verification The mzcompose default sets the lookahead, so the existing mzcompose-based MySQL source tests exercise the early rewind path. #40208 adds targeted coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <[email protected]>
The storage persist sink has been updated to support grouping data into a single batch by committing to an upper while the the frontier is pinned to the snapshot time. This affected the benchmarks, which detected a ~5s increase in wallclock for tests that covered snapshot.
This PR lowers the CI lookahead to 2s (to make it faster, while still offering coverage). It also moves the version gate from v26.41.0-dev to v26.45.0-dev, the first release containing the dyncfg (#38935). Before this, mzcompose set the parameter on v26.41 to v26.44 images that don't know it, which affects upgrade and platform-check runs. The v26.45 benchmark baseline now runs with the same lookahead as HEAD.
Closes SS-500