Skip to content

[OPIK-7315] [BE] feat(cutover): reverse the Distributed wrap without reversing the cutover - #7992

Open
andrescrz wants to merge 8 commits into
mainfrom
andrescrz/OPIK-7315-unwrap-and-redo-path
Open

[OPIK-7315] [BE] feat(cutover): reverse the Distributed wrap without reversing the cutover#7992
andrescrz wants to merge 8 commits into
mainfrom
andrescrz/OPIK-7315-unwrap-and-redo-path

Conversation

@andrescrz

Copy link
Copy Markdown
Member

Details

Adds rollback.sh --unwrap-only so the Distributed wrap can be reversed on its own, keeping the partitioned successor live, and documents how to retry a cutover after a stage B/C rollback without re-backfilling. Both gaps were found while running the traces cutover and its wrap on real clusters; the runbook and reverse-replay scope of OPIK-7315 already shipped.

  • Un-wrap. Stage C was the only statement touching the wrap, and it bundles four actions (drop the wrapper, promote the parked original, park the successor, reverse-replay). That is disproportionate when only the routing definition is at fault: the wrapper stores no data, yet stage C abandons a validated backfill, makes post-cutover writes non-live, runs the guard-less reverse replay, reverts to the unpartitioned original, and leaves the sentinel/duration repair to do.
  • It is the only wrap recovery left after finalize.sh. Stages B and C both require traces_pre_cutover_backup, which finalize drops. Since the documented order is wrap → soak → finalize, post-wrap-and-post-finalize is the expected steady state — and until now a wrap fault discovered there had no tooling answer at all. It also makes the wrap a switch rather than a one-way door.
  • Retry without re-backfilling. A rollback parks the successor's data and finalize.sh then truncates it, so a retry re-copies a table already copied and verified — minutes vs days at production scale. The parked backup is the same physical object Liquibase created as traces_local_v2 (a ReplicatedMergeTree's replica path is fixed at CREATE and survives renames), so it can be reused. Documented with its guards, deliberately not automated: it reuses data whose trustworthiness may be why the rollback happened, and it revives writes the rollback chose to discard.

Construction is stage C's rename minus the middle clause, so it inherits the same properties: one atomic multi-target RENAME (so traces is never absent on a node) and a DROP that only ever targets the data-less ex-wrapper. No new ClickHouse grantsCREATE/DROP TABLE on traces, traces_local and traces_dist_old is a subset of the stage-C set.

Scope limit, documented in both the SQL header and the runbook: this undoes sharding only. A fidelity defect in the successor, or a partition-count/merge-load/query regression, is a cutover problem — use stage B/C while the parked original still exists.

Change checklist

  • User facing
  • Documentation update

Issues

  • Resolves #
  • OPIK-7315

AI-WATERMARK

AI-WATERMARK: yes

  • Tools: Claude Code
  • Model(s): Claude Opus 5
  • Scope: Full authorship of this change — the new 000004_rollback_unwrap.sql, the --unwrap-only mode and its guards in rollback.sh, the corrected --wrap-only refusal message in exchange_and_wrap.sh, the runbook sections, and the three new cases plus the ON CLUSTER truncate change in TracesLocalV2CutoverTest. Also the investigation that produced them (reading the shipped tooling on main, running the gates, and the mutation/flake analysis below).
  • Human verification: design was operator-directed — both additions were proposed and approved on OPIK-7315 before any code was written, including the explicit decision not to automate the retry path. Gates below were run locally against real containers. The diff itself is pending author review, which is why this is a draft.

Testing

TracesLocalV2CutoverTest — 12 cases, run against dedicated ClickHouse + ZooKeeper testcontainers (JDK 25, the build's target):

export JAVA_HOME=/Library/Java/JavaVirtualMachines/amazon-corretto-25.jdk/Contents/Home
mvn -o surefire:test -Dtest='TracesLocalV2CutoverTest'   # 12/12
mvn -o spotless:check                                    # clean
bash -n data-migrations/traces-local-v2-cutover/scripts/*.sh   # 8/8

Scenarios validated

Case What it pins
unwrapReversesTheWrapKeepingTheSuccessorAndItsPostCutoverWrites The properties that would fail under stage C: post-cutover writes still served, parked original still parked (so B/C stay available). Plus that no data moves — the successor's fidelity fingerprint read through the wrapper beforehand equals the one read off traces after — and why no replay is needed: a post-wrap delete stays deleted because the same table stays live.
unwrapNeedsNoParkedOriginalAndTheWrapCanBeReapplied Un-wrap on a simulated finalized estate (no parked original), then a wrap → un-wrap round-trip with data intact.
rollbackBackupIsReusableAsTheShadowForARetryWithoutRebackfilling The retry procedure end to end, walking the chain step by step (shadow populated → EXCHANGE made it live → rollback parked that copy intact) so a failure localizes itself.

Verified load-bearing, not merely green. Each new case was re-run against the mutation a reader would plausibly make: stubbing unwrap() to a no-op fails the first two (2/2), and replacing the reuse RENAME with finalize.sh's recycle (TRUNCATE + RENAME) fails the third on exactly the reuse assertion. All pass again on restore.

Regressions. Full class green across six consecutive runs. The pre-existing nine cases were unaffected by the @BeforeEach change. Argument matrix exercised by hand — the four --unwrap-only rejections (missing --confirm-maintenance; combined with --stage; combined with --reverse-replay-only; passing any of --cutover-start / --accept-post-cutover-write-loss / --confirm-retention-paused) plus a regression pass confirming the pre-existing stage and reverse-replay paths and --confirm-maintenance's inertness outside un-wrap are unchanged.

One intermittent failure, disclosed. The new reuse test failed on its first full-class run with the parked backup holding 0 of 120 rows, and did not reproduce in five subsequent full runs (three before the fix below, three after), nor in isolation or pair runs. Investigating it found that the three TRUNCATEs ending resetTables were the only DDL there without ON CLUSTER; a bare TRUNCATE on a ReplicatedMergeTree need only be applied locally before the client returns, so the emptying can still be settling while the next test writes, and a late DROP_RANGE can take those rows with it. That mechanism fits every symptom — a zero rather than a partial count, load dependence, and hitting the one test that reads the shadow long after writing it — but it was not reproduced, so this is a principled removal of a plausible cause and a consistency fix, not a demonstrated repair. Recorded so a recurrence reads as "still not fixed"; the step-by-step assertions will localize it.

Not run / not covered. The bash guards (five topology checks, four argument rejections) are outside the automated suite. That is this file's documented scope and a constraint rather than a preference: the drivers need the clickhouse-client binary, which backend_tests.yml does not install, and the repo has no bash test harness — covering them means introducing both. They were exercised by hand as above and belong to the staging rehearsal, as the sibling driver guards do. Multi-shard behaviour and the cross-replica ON CLUSTER skew are also not covered: the harness is single-shard, single-replica.

Documentation

Runbook (data-migrations/traces-local-v2-cutover/README.md):

  • New "Un-wrap: reversing sharding without reversing the cutover" section — when to prefer it over stage C, a cost comparison against stages B/C, the DDL-before-flag ordering and why it inverts the forward direction, which flags stay untouched, and the scope limit.
  • New "Retrying the cutover after a stage B/C rollback" procedure with its four guards.
  • An opening rule for the Rollback section: reverse the smallest thing that fixes the problem — the cutover delivers partitioning and sharding independently, and they roll back separately.
  • Privileges table gains an un-wrap row (needs nothing beyond the stage-C set).
  • Two claims this change made inaccurate are corrected rather than left to contradict it: the "one-way door" wording in the privileges table, and "Point of no return", which now says what finalize does and does not foreclose — it ends the route back to the original, while the wrap stays reversible.

Reviewer note: exchange_and_wrap.sh still refuses --wrap-only when the parked original is gone, so a finalized estate cannot be sharded. Its now-false "one-way door" reasoning is corrected but the refusal is kept — lifting it is a policy decision, not a side effect of this change. Happy to follow up if you want post-finalize wrapping enabled. Stage C also shares the leftover-traces_dist_old exposure that --unwrap-only now pre-checks; left alone to keep this scoped.

🤖 Generated with Claude Code

@github-actions github-actions Bot added documentation Improvements or additions to documentation java Pull requests that update Java code Backend Infrastructure tests Including test files, or tests related like configuration. labels Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

⏱️ pre-commit per-hook timing

Hook Description Result Duration
☕ spotless — java backend Format Java code 4.38s
Total (1 ran) 4.38s
⏭️ 43 skipped (no matching files changed)
Hook Description Result
🐍 trim trailing whitespace — python sdk Strip trailing whitespace ⏭️
🐍 fix end of files — python sdk Ensure files end in a newline ⏭️
🐍 ruff — python sdk Lint + autofix Python (ruff) ⏭️
🐍 ruff-format — python sdk Format Python code (ruff) ⏭️
🐍 mypy — python sdk Static type check ⏭️
🤖 trim trailing whitespace — optimizer Strip trailing whitespace ⏭️
🤖 fix end of files — optimizer Ensure files end in a newline ⏭️
🤖 check yaml — optimizer Validate YAML syntax ⏭️
🤖 check json — optimizer Validate JSON syntax ⏭️
🤖 check toml — optimizer Validate TOML syntax ⏭️
🤖 check for added large files — optimizer Block large files (>1MB) ⏭️
🔐 detect private key — optimizer Block committed private keys ⏭️
🤖 check for merge conflicts — optimizer Block merge-conflict markers ⏭️
🤖 check for case conflicts — optimizer Block case-only name clashes ⏭️
🤖 pyupgrade — optimizer Modernize Python syntax ⏭️
🤖 ruff — optimizer Lint + autofix Python (ruff) ⏭️
🤖 ruff-format — optimizer Format Python code (ruff) ⏭️
🤖 mypy — optimizer Static type check ⏭️
📓 nbstripout — optimizer notebooks Strip notebook output ⏭️
📝 markdownlint — optimizer Lint Markdown ⏭️
🔤 codespell — optimizer Fix common misspellings ⏭️
📊 radon cc — optimizer Cyclomatic-complexity gate ⏭️
📊 radon raw — optimizer Raw size metrics gate ⏭️
📊 xenon — optimizer Fail on complexity thresholds ⏭️
📊 lizard — optimizer Cyclomatic-complexity gate ⏭️
🧹 vulture — optimizer Find dead code ⏭️
🛡️ trim trailing whitespace — guardrails Strip trailing whitespace ⏭️
🛡️ fix end of files — guardrails Ensure files end in a newline ⏭️
🛡️ ruff — guardrails Lint + autofix Python (ruff) ⏭️
🛡️ ruff-format — guardrails Format Python code (ruff) ⏭️
🛡️ mypy — guardrails Static type check ⏭️
⚓ helm-docs Regenerate Helm chart README ⏭️
block non-public FE plugins Block non-public FE plugins ⏭️
🧪 pre-commit wrapper smoke tests Self-test the wrapper scripts ⏭️
🧪 rebaseline script tests Self-test the changelog re-baseline script ⏭️
🌐 eslint — frontend Lint + autofix JS/TS ⏭️
🌐 typecheck — frontend Whole-project tsc type check ⏭️
📘 eslint — typescript sdk Lint + autofix JS/TS ⏭️
📘 typecheck — typescript sdk Whole-project tsc type check ⏭️
⚙️ actionlint — github workflows Lint GitHub Actions workflows ⏭️
🐳 hadolint — dockerfiles Lint Dockerfiles ⏭️
🌈 zizmor — github workflows security Security-scan GitHub Actions workflows ⏭️
🛡️ semgrep — java backend sql Block SQL injection-prone string formatting ⏭️

Comment thread apps/opik-backend/data-migrations/traces-local-v2-cutover/scripts/rollback.sh Outdated
Comment thread apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md
@andrescrz
andrescrz force-pushed the andrescrz/OPIK-7315-unwrap-and-redo-path branch from 93b513c to 2f60732 Compare August 25, 2026 13:15
@andrescrz
andrescrz force-pushed the andrescrz/OPIK-7315-unwrap-and-redo-path branch from 2f60732 to cdbca4a Compare August 25, 2026 15:16
@comet-ml comet-ml deleted a comment from github-actions Bot Aug 25, 2026
@comet-ml comet-ml deleted a comment from github-actions Bot Aug 25, 2026
Comment thread tests_load/tests/traces-local-v2-cutover/README.md Outdated
Comment thread apps/opik-backend/data-migrations/traces-local-v2-cutover/scripts/verify.sh Outdated
andrescrz and others added 5 commits August 25, 2026 18:12
…reversing the cutover

Two additions to the shipped rollback tooling, both found while running the traces cutover and its wrap on
real clusters. The runbook and reverse-replay scope of this ticket already shipped; these are the gaps that
remained.

1. `rollback.sh --unwrap-only`, backed by a new `000004_rollback_unwrap.sql`.

Stage C was the only statement that touched the wrap, and it bundles four actions: drop the wrapper, promote
the parked original, park the successor, reverse-replay. That is right when the successor is suspect and
disproportionate when it is not — the wrapper stores no data, yet reversing it via stage C abandons a validated
backfill, makes post-cutover writes non-live, runs the guard-less reverse replay, reverts to the unpartitioned
original, and leaves the sentinel/duration repair to do. The wrap is also the newest and least-exercised half
of the cutover, so it is the half most likely to need backing out.

Keeping the successor live has two consequences:

* No reverse-replay, and none of its ceremony. Nothing is promoted, so no write is abandoned and no bridged
  delete needs re-applying. `--cutover-start`, `--accept-post-cutover-write-loss` and
  `--confirm-retention-paused` are therefore rejected rather than ignored: each asserts a precondition for
  something this does not do, and silently accepting one would confirm a wrong mental model — most damagingly
  that post-cutover deletes get replayed.
* It works after `finalize.sh`. Stages B and C both require `traces_pre_cutover_backup`, which finalize drops;
  this needs only `traces` and `traces_local`. Since the documented order is wrap, soak, then finalize,
  post-wrap-and-post-finalize is the expected steady state — and until now a wrap fault discovered there had
  no tooling answer at all. That gap is the main reason this exists. It also makes the wrap a switch rather
  than a one-way door: `--wrap-only` applies it, `--unwrap-only` removes it, repeatably.

Construction is stage C's rename minus the middle clause, so it inherits the same properties: a single atomic
multi-target RENAME means `traces` is never absent on a node, and the only DROP targets the ex-wrapper under
`traces_dist_old`, a name only the data-less wrapper ever occupies. It needs no new privileges — CREATE/DROP
TABLE on `traces`, `traces_local` and `traces_dist_old` is a subset of the stage-C grant set.

Guards: `traces` must be Distributed, `traces_local` must exist and must carry the successor schema, and
`traces_dist_old` must be free. The schema check is not redundant — promoting an original that some earlier
manual step left under `traces_local` would revert the schema with none of the flag reverts or sentinel repair
a real rollback performs. The `traces_dist_old` check turns a bare "table already exists" from the rotate into
a diagnosis with its one-line remediation; it is reachable when an earlier stage C left the name behind and a
re-wrap followed.

Flag ordering is the inverse of the forward direction, deliberately. Un-wrap first, then revert
`tracesDistributedWrapEnabled`: that leaves deletes pointed at the now-absent `traces_local` (Code 60) until
the roll-restart lands, whereas reverting the flag first would point them at a still-Distributed `traces`,
which rejects mutations (Code 36). Both flag windows are delete-path-only, since TraceDAO reads the flag only
when choosing its mutation table. The DDL window is separate and is NOT delete-only: while the ON CLUSTER
rename propagates, a lagging replica still resolves the wrapper's `traces_local` target, which the
already-renamed replicas no longer have, so a query routed there can fail with UNKNOWN_TABLE — the exact
mirror of the wrap's own window. Because that touches reads, the async-insert buffer alone does not cover it;
`--confirm-maintenance` asserts quiesced traffic or a maintenance window. `traceColumnsNonNullable` stays true
and `tracesWeeklyPartitionPruningEnabled` stays as it was: the live table is still the partitioned,
sentinel-schema successor, which is what both flags assert.

Scope limit, documented in both the SQL header and the runbook: this undoes sharding only. A fidelity defect
in the successor, or a partition-count, merge-load or query regression, is a cutover problem that un-wrapping
does not touch — use stage B/C while the parked original still exists.

Corrects one now-inaccurate message: `exchange_and_wrap.sh` refused `--wrap-only` without a parked original on
the grounds that it "makes the wrap one-way". The refusal is kept — wrapping an estate with no path back to
the pre-cutover table should stay a deliberate, separately reviewed act — but the stated reason was wrong once
the wrap became independently reversible, so it now names what is actually missing (stage B/C).

2. The runbook's "retry after a rollback without re-backfilling" procedure.

A stage B/C rollback parks the successor's data, and the documented next step (`finalize.sh`) truncates it —
so a retry re-copies a table that was already copied and verified. At production scale that is the difference
between minutes and days. The copy can be reused, because the parked backup is the same physical object
Liquibase created as `traces_local_v2`: a ReplicatedMergeTree's replica path is fixed at CREATE and survives
renames, the same property finalize's own recycle branch relies on.

Documented, deliberately not automated. It reuses data whose trustworthiness may be exactly why the rollback
happened, and it revives writes the rollback chose to discard — correct in one scenario and wrong in others,
which is a poor fit for a script, and it is a single RENAME. The runbook names the guards instead: finalize
must not have run; the rename fails if a shadow already exists (itself the signal that a retry began before
the rollback was finalized); the discarded post-cutover writes come back; and the retry resumes the normal
sequence, delta from the ORIGINAL anchor then verify before the EXCHANGE — bounded to the sealed weeks,
because the reused shadow is a superset of the restored original by exactly those revived writes, so an
unbounded compare reports them and reads as a fidelity failure on a good retry.

TESTS (`TracesLocalV2CutoverTest`, now 12 cases)

* `unwrapReversesTheWrapKeepingTheSuccessorAndItsPostCutoverWrites` asserts what would FAIL under stage C —
  post-cutover writes still served, and the parked original still parked, so stage B/C stay available. It
  pins that no data moves, comparing the successor's fidelity fingerprint read through the wrapper beforehand
  against the one read off `traces` after, and pins why no replay is needed: a post-wrap delete stays deleted
  because the same table stays live.
* `unwrapNeedsNoParkedOriginalAndTheWrapCanBeReapplied` simulates a finalized estate, un-wraps with no parked
  original, then round-trips wrap -> un-wrap with the data intact.
* `rollbackBackupIsReusableAsTheShadowForARetryWithoutRebackfilling` covers the second addition end to end and
  walks the chain step by step (shadow populated -> EXCHANGE made it live -> rollback parked that copy intact),
  so a failure localizes itself. Writing it is what surfaced the bounded-verify correction above.

All three were verified load-bearing rather than merely green, each with the mutation a reader would plausibly
make: stubbing `unwrap()` to a no-op fails the first two, and replacing the reuse RENAME with finalize's
recycle (TRUNCATE + RENAME) fails the third on exactly the reuse assertion.

The reset's three TRUNCATEs now use ON CLUSTER, like every other DDL in that method and like stage A's
truncate. On a ReplicatedMergeTree a bare TRUNCATE need only be applied by the local replica before the client
returns, whereas ON CLUSTER makes the client wait for the distributed DDL task — a real barrier before a test
body starts writing. This is the best-supported explanation for a flake seen once: the new reuse test failed
on its first full-class run with the parked backup holding 0 of 120 rows, and did not reproduce in five
subsequent full runs (three before this change, three after) or in isolation and pair runs. Stated plainly —
the mechanism fits every symptom (a zero rather than a partial count, load dependence, and hitting the one
test that reads the shadow long after writing it) but it was NOT reproduced, so this is a principled removal
of a plausible cause and a consistency fix, not a demonstrated repair.

Coverage boundary, on the record rather than implied: the bash guards (five topology checks and four argument
rejections) stay outside the automated suite, which is this file's documented scope and a constraint rather
than a preference — the drivers need the `clickhouse-client` binary, which `backend_tests.yml` does not
install, and the repo has no bash test harness, so covering them means introducing both. They were exercised
by hand (the full argument matrix plus a regression pass over the pre-existing stage and reverse-replay paths)
and belong to the staging rehearsal, as the sibling guards do.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…retry paths

Six findings from the automated review; four accepted as real defects, two taken as wording, one declined with
reasoning (below). Each was checked against the code before judging it — two turned out to be genuine bugs in
what I added, and one exposed a pre-existing inaccuracy in the sibling driver.

1. THE UN-WRAP TOLD OPERATORS TO RUN A COMMAND THAT CANNOT WORK. Its closing message ended with an
   unconditional "to re-apply the wrap later: exchange_and_wrap.sh --wrap-only …", but that driver refuses
   while `traces_pre_cutover_backup` is absent — which is precisely the post-finalize estate un-wrap exists to
   serve. So the one case that motivated this mode was the one where the follow-up instruction was guaranteed
   to fail, and the runbook called the wrap "a switch … repeatably" without qualifying it. The message is now
   guard-aware: it prints the command when the parked original is present, and when it is not, says so and
   points at the runbook rather than handing over a certain refusal. The runbook records the asymmetry —
   un-wrap stays repeatable, re-wrapping a finalized estate needs the guard lifted first, which is a separate
   reviewed decision.

   The review also caught that the finalized-estate test re-wraps via the SQL helper, not the driver, so it
   cannot observe that refusal. Correct: that is driver scope this suite excludes. Noted in the test's Javadoc
   so "re-appliable" is not read as a claim about the CLI.

2. THE RETRY PATH OMITTED A PREREQUISITE, AND THE FAILURE IS SILENT. The rollback tells the operator to set
   `traceColumnsNonNullable` back to `false`; the retry then puts the sentinel-schema successor back under
   `traces`, which needs it `true` again with every instance restarted. The procedure never said so — four
   steps went rename, delta, verify, EXCHANGE — and skipping it does not fail loudly: absent `end_time` reads
   back as `1970-01-01` while writes keep succeeding. Now an explicit step, with the buffer raise, and with
   `tracesWeeklyPartitionPruningEnabled` called out as separate (safe to lag, unsafe to lead).

   Also recorded why `verify.sh` cannot cover for it: it normalizes NULL and the epoch sentinel to the same
   fingerprint (`coalesce(..., 0)` on one side, `toUnixTimestamp64Micro(epoch) = 0` on the other, verified), so
   it passes either way. Only a positive read-back check catches a missed flip.

3. UN-WRAP COULD LEAVE THE LWD GAUGE SILENTLY EMPTY. At wrap time the runbook offers pointing
   `PARTITION_METRICS_LWD_TABLES` at `traces_local` "for label consistency". If that option was taken, the
   un-wrap removes the table it names, so the scan fails with Code 60 and
   `opik.clickhouse.partition.lwd_rows` empties while every other gauge returns. The un-wrap guidance covered
   the parts-gauge relabel and missed this. Now in both the runbook and the closing message, conditioned on
   having taken the option — the default (`traces,spans`) needs nothing. (The review implied the override
   points at `traces_local` by default; it does not, which is why this is conditional rather than universal.)

4. A MISSING `end_time` PASSED THE SUCCESSOR CHECK. The guard read
   `[[ "$(traces_endtime_type traces_local)" != Nullable* ]]`, and that helper returns an empty string for a
   column that does not exist — which is not `Nullable*`, so it passed. A table with no `end_time` at all
   would have been promoted to live `traces`. Now existence is required first, then non-nullability.

5. `--confirm-maintenance` WAS DOCUMENTED AS A WRITE-SIDE GATE ON BOTH SIDES. The review is right that an
   async-insert buffer protects writes and not reads, so acknowledging the buffer alone should not discharge
   the flag. Widened to say the exposure is a read one too, and that quiescing traffic or a window is what the
   flag asserts. The same correction lands in `exchange_and_wrap.sh`: its `--confirm-maintenance` text and
   refusal message described only the buffer, which understates the wrap's own window in exactly the same way
   — a pre-existing inaccuracy this change surfaced by mirroring it.

   Not taken: a separate read-quiescence flag. These two operations are inverses and share an operator; giving
   them differently-named confirmations for the same physical window would fragment the interface without
   adding safety, since neither assertion is verifiable by the script.

6. THE "DISCARDED WRITES COME BACK" CLAIM HAD AN EXCEPTION. An id deleted and then re-created after
   `cutover_start` does not come back on a reuse-retry: the rollback's reverse replay masked it on the restored
   original (deliberately guard-less), so the forward replay's resurrection guard then sees it as not-live on
   the source and masks it on the shadow too. Documented as the exception rather than fixed — the guard is
   correct for the primary cutover path, and rewriting shared replay SQL for an edge case inside an edge case
   would put the main path at risk to serve a rare one.

DECLINED, with reasoning: adding a CI harness that executes `rollback.sh` to assert its exit codes. The finding
is accurate — no repository test invokes any of these drivers — but it is a constraint rather than an
oversight: the drivers need the `clickhouse-client` binary, which `backend_tests.yml` does not install, and
there is no bash test harness in the repo, so this means new CI infrastructure. It also applies equally to all
seven pre-existing drivers, so this change does not worsen the ratio, and the review itself asks to keep the
gate test's SQL-only scope, which concedes the split. The argument matrix and topology rejections were
exercised by hand and belong to the staging rehearsal, as the sibling guards do. Worth its own ticket if we
want driver-level coverage.

Also declined: cluster-wide `engine_full` validation of the wrapper (its cluster, target and sharding key). The
un-wrap drops that wrapper, so its routing definition cannot affect the outcome; and stage C does not do
cluster-wide preflight either, while `ON CLUSTER` is synchronous and throws naming a laggard. If we want
`clusterAllReplicas` rigor on the live-`traces` renames it should land for both stages together rather than
diverging here.

Gates: full class 12/12, spotless clean, all eight drivers parse, and the un-wrap argument matrix plus the
three end_time guard states (missing / Nullable / successor) re-checked.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…non-vacuous

Rehearsing the whole procedure locally — forward cutover, wrap, stages A/B/C,
the un-wrap (before and after finalize), the retry path and both finalize
branches — surfaced three problems in verify.sh, the gate the runbook points
operators at, plus stale guidance in the load-harness notes.

verify.sh --from-week/--to-week are 0-based week OFFSETS from an anchor derived
from the old table's created_at, but five call sites wrote the bound as
"<last sealed week>", which reads as a date. Pasting a YYYYMMDD partition name
passed the integer check and then walked millions of empty windows: no error, no
progress, no result. Add 'last-sealed' so the bound the runbook actually wants is
expressible directly, reject a --to-week past the last week with data — bounding
by the data needs no threshold and cannot reject an offset the table can serve,
while far-future ids never inflate one, since the anchor comes from created_at —
and say OFFSET in the help.

verify.sh also reported "PASSED: all 0 windows match" when the bounds selected no
window — a vacuous pass from a fidelity gate. It now fails, since reaching that
point means nothing was compared, not that the data agrees.

The load-harness notes claimed chaining rollback stages needs finalize.sh
--confirm or a fresh volume; the retry path removes both. They also made
countIf(duration < 0) reaching 0 the sentinel repair's success criterion, which
holds only because locally generated traces never end before they start — on a
real table that count has a non-zero floor and would read a correct repair as a
failed one. Document the sentinel counters as the criterion, and add the
--unwrap-only rehearsal the harness had no coverage for.

Also gitignore backfill.sh's anchor state file: it is written to the CWD, so a run
from the repo root leaves it one `git add -A` away from being committed.
…d, not the newest populated one

'last-sealed' resolved to LAST_WEEK - 1, where LAST_WEEK is the week holding
max(created_at). With ingestion live those coincide, but on a quiet table the
newest row can already sit in a sealed week - and subtracting one then skipped
that week, so the most recently written data went uncompared and a divergence
there was reported as PASSED. Same silent-narrowing class as the empty-range
vacuous pass. A table holding exactly one already-sealed week was worse: it
resolved to -1 and refused to verify at all.

Resolve against the current CALENDAR week instead - the only week that can still
change - capped at LAST_WEEK so a quiet table still verifies everything it holds.
now('UTC') matches created_at's own timezone, so the boundary agrees with the
anchor even where the server timezone is not UTC. Behaviour under live ingestion
is unchanged, and the "no sealed week" guard still covers a table whose data is
entirely in the current week.

Also give the harness notes the bounded post-swap invocation instead of only
describing it. The primary command stays UNBOUNDED on purpose: the current week is
where the delta and the final deletion replay land, so it is the week most worth
comparing, and the step is run while writes are still held. Bounding it there
would permanently skip the riskiest week.
…over_start, and correct what a sealed-week mismatch means

Two problems with pointing the post-rollback and retry compares at
'--to-week last-sealed'.

It bounds by the calendar, but the divergence is bounded by the cutover window.
Those are the same week only while the verify runs promptly; run it in any later
week and the window's own week counts as sealed, so the writes the rollback
deliberately discarded read as a fidelity failure. rollback.sh now computes and
prints the offset of the last week wholly before cutover_start, which does not
drift, and the runbook points at that instead of deriving one by hand. The query
is advisory and runs after the rollback has already succeeded, so it is
non-fatal: a blip prints instructions rather than aborting and swallowing the
remaining steps.

The stated rule - that a mismatch in a sealed week is the real signal - was also
too strong, in three places. Any write touching a PRE-EXISTING trace after
cutover_start diverges it in a sealed week, which no weekly bound can exclude,
because the divergence sits where the row was born rather than where the write
happened. Two shapes, confirmed locally: the trace-update endpoint keeps the
row's created_at, so the key differs on both sides; batch ingestion re-stamps it,
so the key goes missing from the successor in its original week. Both are the
discarded-write class. Document the triage that separates them from a real copy
gap - look the differing ids up in the successor without a week filter, and treat
last_updated_at >= cutover_start as benign - and carry the same caveat into the
post-EXCHANGE note, which asserted the same rule.
@andrescrz
andrescrz force-pushed the andrescrz/OPIK-7315-unwrap-and-redo-path branch from 9f910b2 to 1ac4f20 Compare August 25, 2026 16:12
Comment thread apps/opik-backend/data-migrations/traces-local-v2-cutover/scripts/rollback.sh Outdated
Comment thread apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md Outdated
Comment thread apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md
…ke the flag probe discriminate

Two things the previous round got wrong.

The bound rollback.sh prints was derived from min(created_at) and cutover_start
alone, so an environment idle for a week or more before the cutover produced an
offset past verify.sh's own last populated week - which verify.sh then rejects,
making the emitted command exit without comparing anything. Cap it at that
offset. The cap only binds when the window's week is already beyond the last
populated one, and in that case the window is out of range anyway, so the
divergence stays excluded either way.

The traceColumnsNonNullable rollout was to be verified by writing an in-progress
trace and asserting it reads back null. That check cannot fail before the
EXCHANGE: the live table is still Nullable, so a stale-false instance stores NULL
and a live-true instance stores the sentinel, and both read back as absent -
confirmed locally, identical API responses either way. What discriminates before
the swap is what was WRITTEN (sentinel vs NULL); the read-back only discriminates
after it, once the column is non-nullable and a stale instance serves 1970 for an
absent value. Say so at all three places that prescribe the probe, and require
the post-EXCHANGE run explicitly on the retry path, which had no post-swap check
at all.
…documentation

Pass over every comment, javadoc and runbook section this branch touches.

Two were wrong rather than merely wordy. verify.sh's --to-week help still
described 'last-sealed' as stopping before the newest week "and what a
post-rollback compare wants": it stops before the current CALENDAR week, and the
post-rollback compare should be bounded by cutover_start, which rollback.sh now
prints. Its --old-table entry recommended the same token and so contradicted both
the runbook and the driver. The load-harness notes had the matching stale framing
("bound it to the sealed weeks, because the current week legitimately diverges").

The rest is trimming to what a future reader needs. Comments that argued against
approaches this branch did not keep, or narrated a defect the code no longer has,
say the invariant instead: the week bound is explained by what it must exclude,
not by what an earlier version excluded. The un-wrap SQL header drops its
duplicate of the runbook's stage-C comparison and points there. The gate test's
ON CLUSTER note no longer asserts a diagnosed cause for a flake that was never
reproduced — it states why the barrier is the safe form and calls the failure
shape what it is.

No confidential data, no private-document references, and no environment or
customer identifiers in any of it; the estate-specific figure ("~20M windows")
that came from one operator's mistyped bound is now stated as a magnitude.
…rollback.sh prints, and probe both flag arms

rollback.sh takes --host/--port and threads them through every clickhouse-client
command it prints, but the three driver invocations added on this branch dropped
them: the post-rollback verify (both branches) and the re-wrap. Copied as printed,
they fall back to clickhouse-client's defaults - and since it honors
CLICKHOUSE_HOST only when no connection flag is given, an operator reaching the
cluster through a port-forward would silently verify localhost, or the wrong
cluster. Thread the values through as the other printed commands already do.

The traceColumnsNonNullable probe asserted end_time alone. The flag governs a
second arm, ttft, whose absent value is NaN rather than the epoch, and a trace
written without an end_time has no ttft either - so one probe trace covers both
and there is no reason to check only half of what the flag switches. Assert both,
at each place the probe is prescribed.
@comet-ml comet-ml deleted a comment from github-actions Bot Aug 25, 2026
@andrescrz
andrescrz marked this pull request as ready for review August 25, 2026 17:52
@andrescrz
andrescrz requested review from a team as code owners August 25, 2026 17:52
@CometActions

Copy link
Copy Markdown
Collaborator

Already covered by a test in this PR.

The un-wrap path ships with its own gate tests in this PR. TracesLocalV2CutoverTest.unwrapReversesTheWrapKeepingTheSuccessorAndItsPostCutoverWrites asserts the wrapper is gone, traces is still the SUCCESSOR (non-Nullable end_time), traces_local/traces_dist_old are both freed, the fidelity fingerprint read through the wrapper equals the one read off traces after, post-cutover writes are still live, the post-wrap delete stays deleted, and traces_pre_cutover_backup is untouched; unwrapNeedsNoParkedOriginalAndTheWrapCanBeReapplied covers the finalized estate and the wrap -> un-wrap -> wrap round-trip. Any of those fails if 000004_rollback_unwrap.sql's rotate is wrong, so there is nothing for the e2e estate to add. The rest of the diff is runbook drivers, README and .gitignore under data-migrations/, which no Playwright spec can drive.

Also considered. The bash-driver half is uncovered but is not an e2e gap. verify.sh's checked == 0 fix is the highest-consequence change in the diff — a bounded compare previously printed PASSED having compared no window at all — and it sits alongside the new --to-week out-of-range rejection, 'last-sealed', and rollback.sh's --unwrap-only guards (Distributed check, traces_local end_time existence-then-nullability order, leftover traces_dist_old pre-check). Nothing automated exercises any of them, and the suite's own Javadoc says so deliberately: it validates the cutover SQL, not the drivers, which belong to the OPIK-6901 staging dry-run. The Playwright estate cannot run a migration driver against a multi-node ClickHouse cluster, so this is not a deferral waiting on a gate to open — it is work for the staging rehearsal. Worth confirming that rehearsal exercises at least one empty week range, since that is the case the vacuous-pass fix exists for.

also touches Backend (Java API / internal)

Run

Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backend baz: pending documentation Improvements or additions to documentation Infrastructure java Pull requests that update Java code 🔴 size/XL tests Including test files, or tests related like configuration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants