Skip to content

Commit 35c8173

Browse files
andrescrzclaude
andcommitted
fix(cutover): resume only on dst>=src; residual full-compare gate; test hygiene
Address Baz round (items 1-5): - backfill.sh (#916, high): resume skips a window only when dst >= src. DIVERGENCE is no longer a resume criterion — a partially-copied window within tolerance would be skipped and its pre-anchor rows lost forever (delta only re-copies at/after backfill_start). The INSERT is idempotent, so re-copying a short window is safe; DIVERGENCE still governs the post-copy abort. - README (#666, high): the pre-EXCHANGE fidelity gate must be a full compare (--sample-mod 1 --weeks-stride 1, no week narrowing); a single cross-project workspace-scoped residual row can be hashed out or week-skipped by sampling. - delete_traffic.py (#890): comment referenced newest 500; now references REFILL_FETCH. - TracesLocalV2CutoverTest (#901): wrapInDistributed() uses a text block + .formatted(). - _common.py (#922): delete the cutover-anchor trace in a finally block so discovery leaves no extra live trace skewing seeded counts. Gate test 10/10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 80df6c0 commit 35c8173

5 files changed

Lines changed: 42 additions & 24 deletions

File tree

apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -629,8 +629,11 @@ cheap (stage A); the bridge stays enabled so nothing is lost on a retry.
629629
- [ ] **Schema-parity guards green**`cutoverCopiesEveryBaseColumn` and `successorMaterializedColumnsMatchSource` pass
630630
on the release, so the cutover copies every base column of `traces` and the two tables' base and materialized
631631
columns match.
632-
- [ ] **Fidelity verified**`verify.sh` passes (full, or a documented representative sample) between source and
633-
destination before the EXCHANGE. Re-run `delta_replay.sh` then `verify.sh` until it PASSES: while the buffer holds
632+
- [ ] **Fidelity verified**`verify.sh` passes between source and destination before the EXCHANGE. This gate MUST be a
633+
**full compare** (`--sample-mod 1 --weeks-stride 1`, no `--from-week`/`--to-week` narrowing): a cross-project
634+
workspace-scoped residual row is a single key, so any sampling (`--sample-mod > 1`), week stride, or week narrowing
635+
can hash it out or skip its week and still report `ok=1`. Reserve sampling/ranged runs for follow-up confidence
636+
*after* the full gate passes. Re-run `delta_replay.sh` then `verify.sh` until it PASSES: while the buffer holds
634637
writes (or, on a rehearsal without it, once traffic is quiescent) the last delta must catch every in-flight write.
635638
- [ ] **`Distributed` wrap gated on app-readiness** — apply the wrap (step 4, part 2) only when the delete/read DAOs are
636639
sharding-aware; otherwise stop after the `EXCHANGE`, since a lightweight `DELETE` against a `Distributed` `traces`

apps/opik-backend/data-migrations/traces-local-v2-cutover/scripts/backfill.sh

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,9 +210,13 @@ insert_window() {
210210
src="$(count_src_uniq "$lo" "$hi")"
211211
dst="$(count_dst_uniq "$lo" "$hi")"
212212

213-
# Resume: destination already has this window (exact, ahead due to concurrent deletes, or within tolerance).
214-
if [[ "$dst" != "0" ]] \
215-
&& [[ "$(awk -v s="$src" -v d="$dst" -v p="$DIVERGENCE" 'BEGIN { print (d >= s || (s - d) / s <= p) ? 1 : 0 }')" == "1" ]]; then
213+
# Resume: skip only when the destination already holds at least as many logical rows as the source (exact, or ahead
214+
# because concurrent deletes shrank the source). DIVERGENCE is NOT a resume criterion: a partially-copied window can
215+
# sit a hair short of src yet within tolerance, and skipping it would leave those rows missing forever — the delta
216+
# step only re-copies rows at/after backfill_start, so a pre-anchor gap is unrepairable. The backfill INSERT is
217+
# idempotent (ReplacingMergeTree, mask-honoring), so re-copying a short window is safe and cheap. DIVERGENCE governs
218+
# only the post-copy abort below.
219+
if [[ "$dst" != "0" && "$dst" -ge "$src" ]]; then
216220
log "$label ($lo .. $hi): already present (src_uniq=$src dst_uniq=$dst), skipping"
217221
return
218222
fi

apps/opik-backend/src/test/java/com/comet/opik/infrastructure/TracesLocalV2CutoverTest.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -987,10 +987,11 @@ private void exchangeTables() {
987987
// need not exist yet), then one atomic multi-target RENAME rotates the data to traces_local and the wrapper into
988988
// traces (the name freed by the first clause), so traces is never absent on a node.
989989
private void wrapInDistributed() {
990-
execute(("CREATE TABLE traces_dist ON CLUSTER '{cluster}' AS traces "
991-
+ "ENGINE = Distributed('{cluster}', '" + DATABASE_NAME + "', 'traces_local', sipHash64(project_id))"),
992-
_ -> {
993-
});
990+
execute("""
991+
CREATE TABLE traces_dist ON CLUSTER '{cluster}' AS traces
992+
ENGINE = Distributed('{cluster}', '%s', 'traces_local', sipHash64(project_id))
993+
""".formatted(DATABASE_NAME), _ -> {
994+
});
994995
execute("""
995996
RENAME TABLE
996997
traces TO traces_local,

tests_load/tests/traces-local-v2-cutover/_common.py

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -62,15 +62,25 @@ def discover_workspace_and_project(
6262
opik_client.trace(id=anchor_id, name="cutover-anchor", project_name=project_name, input={"anchor": True}).end()
6363
opik_client.flush()
6464

65-
deadline = time.time() + timeout_s
66-
while time.time() < deadline:
67-
rows = ch.query(
68-
"SELECT workspace_id, toString(project_id) FROM traces WHERE id = {id:String} LIMIT 1",
69-
parameters={"id": anchor_id},
70-
).result_rows
71-
if rows:
72-
workspace_id, project_id = rows[0]
73-
LOGGER.info("Resolved project '%s': workspace_id=%s project_id=%s", project_name, workspace_id, project_id)
74-
return workspace_id, project_id
75-
time.sleep(0.5)
76-
raise TimeoutError(f"anchor trace for project '{project_name}' did not appear in ClickHouse within {timeout_s}s")
65+
# Always remove the anchor: leaving it behind would add one live trace to the target project and skew seeded
66+
# counts and delete/cutover verification. Cleanup failures are logged, not raised, so they don't mask a real error.
67+
try:
68+
deadline = time.time() + timeout_s
69+
while time.time() < deadline:
70+
rows = ch.query(
71+
"SELECT workspace_id, toString(project_id) FROM traces WHERE id = {id:String} LIMIT 1",
72+
parameters={"id": anchor_id},
73+
).result_rows
74+
if rows:
75+
workspace_id, project_id = rows[0]
76+
LOGGER.info(
77+
"Resolved project '%s': workspace_id=%s project_id=%s", project_name, workspace_id, project_id
78+
)
79+
return workspace_id, project_id
80+
time.sleep(0.5)
81+
raise TimeoutError(f"anchor trace for project '{project_name}' did not appear in ClickHouse within {timeout_s}s")
82+
finally:
83+
try:
84+
opik_client.rest_client.traces.delete_traces(ids=[anchor_id])
85+
except Exception as exc: # noqa: BLE001
86+
LOGGER.warning("could not delete cutover-anchor trace %s: %s", anchor_id, exc)

tests_load/tests/traces-local-v2-cutover/delete_traffic.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,9 @@ def main(project, tps, duration, batch):
7777
continue
7878
pool.extend(fetched)
7979
if not pool:
80-
# An empty refill can be transient: the delete mask may not be visible to search yet, so the newest 500
81-
# ids can all still be in `seen`/`pool`. Only stop after several consecutive empty refills, so a mask-lag
82-
# blip doesn't end the run while thousands of lower-id traces remain undeleted.
80+
# An empty refill can be transient: the delete mask may not be visible to search yet, so the newest
81+
# REFILL_FETCH ids can all still be in `seen`/`pool`. Only stop after several consecutive empty refills,
82+
# so a mask-lag blip doesn't end the run while thousands of lower-id traces remain undeleted.
8383
empty_refills += 1
8484
if empty_refills >= EMPTY_REFILL_LIMIT:
8585
LOGGER.info("no more traces to delete after %d empty refills; stopping", empty_refills)

0 commit comments

Comments
 (0)