Reconciler followups - #2402
Draft
shangyian wants to merge 7 commits into
Draft
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
A cube's materialization did not survive a new revision, and on the deployment
path the superseded revision's workflow was not even stopped.
Materializations hang off one specific NodeRevision through
Materialization.node_revision_id, and update_cube_node never migrated them --
a comment there said as much. Availability does not carry over either: it is
scoped to the revision encoded in the materialized table name
(<node>_<version>_<hash>). So after any new cube revision, including a
metadata-only one that earns nothing more than a minor bump, the current
revision has neither a materialization nor availability and the cube silently
falls back to live queries, while the old revision's workflow keeps running and
keeps posting availability for a definition nobody queries any more. A PATCH
changing only a description reproduces it exactly: v1.1 has no materializations,
no availability, and v1.0's materialization is still active.
The rule is now that a new cube revision means rebuilding the materialization
against the new revision and stopping the superseded revision's workflows.
swap_cube_materializations implements it once and both update_cube_node and the
deployment orchestrator's cube path call it, so a cube edited through
PATCH /nodes/{name}/ and the same edit applied through a deploy land on
identical materialization state -- a test asserts that, down to the query
service calls made.
Rebuilt, not copied. A stored config embeds the cube version, the combiner SQL
and a Druid spec, all derived from the old definition, so the new revision goes
back through create_new_materialization and out to schedule_materialization_jobs.
The rebuild reads its intent -- job, schedule, strategy, lookback window -- off
the old revision's persisted Materialization, because for a cube materialized
through the UI that row is the only record of it and there is no YAML to consult.
Cubes that were never materialized are left alone.
The remote half is separated from the DJ-side half so the two can be sequenced
correctly. swap_cube_materializations only mutates and flushes DJ state and
returns the query service work still owed; the caller commits and hands that to
apply_cube_materialization_swap. On the API path the commit falls between the
two, so a slow or failing query service can neither hold the transaction open
nor leave DJ's record of what is active disagreeing with what was requested. On
the deployment path the swaps queue up and are applied only after execute()
commits the outer transaction, which means a deploy that rolls back -- every dry
run, by construction -- has asked the query service to schedule nothing and stop
nothing. Neither remote call may raise: a stop or a schedule that cannot be
delivered must not abort a legitimate cube edit, and the History event records
what was attempted either way. The replacement is scheduled before the old
workflow is stopped, and a rebuild that turns out to be impossible -- the new
revision no longer carries the temporal partition a cube materialization needs,
say -- still stops the old workflow, because DJ has already recorded those
materializations as inactive and leaving their workflow running is the failure
being prevented.
is_non_trivial_cube_change is demoted accordingly. It no longer decides whether
a workflow is stopped, since that now happens on every revision. What is left is
the question its docstring always claimed: whether the previously materialized
table is still usable. The answer rides along on the swap's History event as
previous_table_usable, so an operator can tell whether the rebuild can adopt the
existing data or needs a fresh build and a backfill.
test_trivial_change_keeps_previous_workflows pinned the old behavior, a
description-only change leaving the running workflow alone, and is rewritten as
test_trivial_change_also_swaps_workflows. The assertions in
test_updating_cube_with_existing_cube_materialization,
test_updating_cube_with_existing_materialization and
test_getting_materializations_for_all_revisions that expected the new revision
to have no materializations at all were encoding the same bug and now pin the
rebuilt ones instead.
Groundwork for the reconciler. Three pieces, no deploy behavior yet: `swap_cube_materializations` takes an optional declared block and prefers it over the intent recovered from the superseded materialization, so a push that edits both a metric and the schedule rebuilds once with the new schedule instead of rebuilding with the old one and needing a correction after. The orchestrator can tell a cube that omits `materialization:` from one that sets it to null, via pydantic's `model_fields_set`. Absence means the cube is not managed here and whatever exists is left alone; an explicit null means the cube should not be materialized. `reconcile_declared_materialization` brings a revision in line with its declared block, updating the row of the same name in place because `(name, node_revision_id)` is unique, and reporting whether anything changed so an unchanged re-declare can skip the query service. It compares `schedule` and `strategy` alongside `config`, unlike the upsert endpoint, which compares only `config` -- and since those two are columns rather than config keys, a schedule-only edit there compares equal and is silently dropped.
A cube could declare a schedule, deploy, get status: success, and be running nothing. `materialization:` parsed, validated and round-tripped through `to_spec`, and then no one acted on it. This closes that gap: the block is now reconciled against what is actually materialized on every push. The reconciler iterates every cube in the deployment spec, not the deploy's change list. `materialization` is excluded from `CubeSpec.__eq__` -- configuring a materialization has never cut a node revision, and a YAML-declared schedule has to behave the same way -- so a push that edits only the schedule leaves the cube in the plan's skip list, and one driven off the update list would drop exactly the edit this exists to support. It also means a materialization reconfigured outside YAML is corrected on the next push. Two of the deploy pipeline's fast paths had to widen for the same reason: the plan is empty for a schedule-only push, so the no-changes short-circuit now lets a cube-bearing deployment through. Nothing is torn down by omission. A cube that has a materialization but declares no block keeps it and earns a WARNING result naming both ways out -- `dj pull` to adopt it into YAML, or `materialization: null` to remove it. The materialization may predate the repo or have been set up through the UI, and a push that has not caught up yet must not silently stop a live workflow. Only an explicit null removes anything. An unchanged re-declare costs no query service call at all. The reported operation compares the block against what the cube declared before the deploy, so a dry run says what the wet run will do, while `reconcile_declared_materialization` separately decides whether DJ-side state moved and only then is a schedule requested. That is also what keeps a push editing both a metric and the schedule to one rebuild: the new revision's swap already builds from the declared block, so the reconcile that follows finds the result matching and stands down. A test pins that at one `materialize_cube` call, and another pins that a schedule-only edit still reaches the query service despite being skipped as a node -- reverting either behavior fails them. Remote work is queued onto the existing post-commit drain rather than called inline, so a deploy that rolls back -- every dry run, by construction -- has asked the query service to schedule nothing and stop nothing. A teardown reuses the same drain: a swap with nothing rebuilt and everything superseded is exactly a stop. A cube DJ cannot build for, measures queries at different grains being the case in hand, fails alone with an error naming it and does not take the rest of the deployment with it. Two fixes fall out. `reconcile_declared_materialization` searched for the existing row after the build, but the build sets its own backref and so appends itself to that collection -- it found itself, called it unchanged, and then detached it, dropping the materialization entirely; the collection is now snapshotted first. And `deploy_and_wait` sent every optional field explicitly null, which made a cube that never mentioned `materialization:` look exactly like one that set it to null; it now drops the keys the spec did not declare, as `dj push` does when it posts the YAML as parsed.
Teardown was inferred from the `materialization` key being present and null,
read off pydantic's `model_fields_set`. That distinction does not survive
serialization: `model_dump` emits every optional field explicitly, so a spec
round-tripped through it says `materialization: null` for every cube and, read
that way, asks for every workflow in the namespace to be stopped. `dj push`
posts the YAML as parsed, so nothing did that today, but any caller that
serializes a spec and re-posts it silently requested a mass teardown -- and
`NodeSpec.rendered_spec`, which round-trips through JSON to resolve `${prefix}`,
already destroyed the intent in the other direction.
Removal is now a value, `materialization: none`, so what the cube declared is
what a round trip preserves. Absence, and the null a dump produces from it,
means the cube's materialization is not managed here and whatever exists is left
alone -- with the warning that already names the way out, now naming the
sentinel. A cube that wrote `materialization: null` meaning removal gets that
warning rather than a silent teardown or a silent no-op.
The two alternatives were both worse. Reconstructing `model_fields_set` from the
raw request body does not fix this at all: a serialized spec has the key
present and null in the body too, so presence-based teardown fires exactly as
before -- it only moves where the wrong answer is computed. An `allow_empty`
style opt-in flag gates the footgun without closing it: with the flag set, a
serialized spec still tears down every cube, and turning one materialization off
is a routine edit rather than the namespace-wipe class of mistake that guard
exists for, so the friction buys nothing.
`deployment_payload` in the tests drops its key-pruning workaround and posts a
plain `model_dump`, which is the shape that used to mean mass teardown -- every
deployment test now exercises the round trip, and one asserts directly that a
serialized spec leaves both a declared schedule and an undeclared cube's
materialization exactly as they were.
A cube whose materialization cannot be built raises nothing: every other node still deploys and commits. But the aggregate status derives from per-item statuses, so the FAILED result the reconciler emits is also what makes the deployment `failed`, and the reporting site should say that this is the intended reading rather than an unconsidered choice of status. Reporting `success` for a materialization the author asked for and did not get would recreate the exact silent gap the reconciler exists to close. Behavior unchanged; comment only.
No behavior change. Four collapses: `_apply_declared_spec` was a one-caller wrapper whose only other branch was unreachable and carried a `# pragma: no cover`; it is now three lines inline at its caller, and the pragma is gone. `_declare_cube_materialization` built the same success `DeploymentResult` twice, once for the dry run and once for the wet run. The reconcile is now guarded by `if not self.dry_run` and both paths fall through to a single append, which is what "a dry run says what the wet run will do" is supposed to mean. The rebuild loop's `except Exception` logs with `exc_info=True`, as does the schedule failure. Both are deliberate swallows, so the traceback is the only thing that would tell a genuine bug from the expected case. The materialization tests installed the query service double by hand -- build, override, `try`, `finally: del` -- eleven times over. That is a `mock_qs` fixture now, and `TestDeclaredCubeMaterializations` grows an `upstreams` fixture for the five nodes every one of its cubes is built on, plus helpers for the materialization name and the undeclared-cube warning it was spelling out by hand. Same assertions, ~180 fewer lines, one less level of indentation.
Five combiner tests failed intermittently, on whichever Python version happened
to draw the short straw that run -- 3.12 one day, 3.11 and 3.13 the next. The
whole difference was the optional `AS` before a COALESCE projection alias:
SELECT COALESCE(gg1.date_id, gg2.date_id) AS date_id -- actual
SELECT COALESCE(gg1.date_id, gg2.date_id) date_id -- expected
Those are the same query, and the parser says so: `parse("SELECT x y") ==
parse("SELECT x AS y")` is True, because AST equality does not consider `as_`.
Only `assert_sql_equal` could tell them apart, because it compares re-rendered
strings rather than the trees -- an assertion stricter than the parser's own
notion of sameness, and stricter than SQL's.
Parse-then-re-render now forces `as_` on for projection aliases on both sides,
so the keyword cannot decide a test. Alias *names* still have to match, and CTE
aliases are left alone: their `AS` is not optional, since `WITH gg1 (SELECT ...)`
does not parse.
This makes the assertion robust rather than explaining why generation emits the
keyword inconsistently. That is worth chasing separately -- the projection is
built by `set_alias`, which leaves `as_` as None, and neither `Function` nor
`Alias` renders the keyword by default, so something downstream sets it.
shangyian
force-pushed
the
reconciler-followups
branch
from
August 10, 2026 10:05
c0cfd08 to
9625fa0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan