fix(ingest): report the physical destination table, not the requested name (backend#2895) - #612
Conversation
… name (backend#2895)
saqlainsyed007
left a comment
There was a problem hiding this comment.
Reviewed at 67cc2a85. A real fail-silent-in-the-data-leaving-direction fix; I verified the parser and the fallback. Approving.
What it is (backend#2895, CLI half). data ingest --output-json reported spec["table"] — the operator's --name — but under the cluster's per-ingestion-tables mode the physical table is a ds_<hex> handle and --name is only a catalog label. So a successful ingest emitted a table value naming a table that does not exist: feeding it back to tracebloc data delete fails ("no dataset named …") and data list | grep finds nothing — both signals agree and both are wrong, silent in the direction that leaves data behind. This teaches the CLI to read the destination table the ingestor now prints (pairs with data-ingestors#549) and report that.
The details are right, and I checked them against the code:
submit.SummarygainsDestinationTable, parsed by its own anchored pattern (Destination table:\s*(.+?)\s*$, trimmed) handled beside the ingestor-ID line — deliberately not the trailing-number pattern, since it's an identifier not a statistic, so it isn't miscounted as a numeric field.writePushJSONpreferss.DestinationTableand falls back tospec["table"]when it's nil/empty — so with an older ingestor that doesn't print the line, output is byte-identical to today and the two PRs are safe to merge in either order.- The human panel gains the field (printed only when non-empty), and the string-catalog golden is updated with exactly that one line.
- No per-ingestion-tables flag knowledge leaks into the CLI: the ingestor reports the label when off and the handle when on, so the reported value is correct in both modes with no naming rule restated here — closing the gap with
data delete's own "REAL case-resolved spelling" standard.
PR reports five mutation-proved tests (each with the mutation that reddens it). CI green, MERGEABLE, no open threads. LGTM.
Part 2 of tracebloc/backend#2895. Pairs with tracebloc/data-ingestors#549, which makes the ingestor print the destination table; this teaches the CLI to read and report it.
Safe to merge in either order. With an ingestor that does not print the line, the field stays empty and output is byte-identical to today's.
The problem
--output-jsonreportedspec["table"]— the operator's--name. When the cluster runs per-ingestion tables the physical table is ads_<hex>handle and the--nameis only a catalog label, so a successful ingest emitted:{ "status": "succeeded", "table": "dropcheck_train", "inserted_records": 30000 }…naming a table that does not exist. Feeding that value straight back to
tracebloc data deletefails with "no dataset named …", andtracebloc data list | grep <name>finds nothing either — which reads as already cleaned up. Both signals agree and both are wrong, so the failure is silent in the direction that leaves data behind.data deletealready holds itself to the right standard —data_delete.go:312: "the REAL (case-resolved) spelling, not the raw argument". Ingest did not, and the two disagreed.The change
submit.SummarygainsDestinationTable, parsed from the banner besideIngestor ID(an identifier line, not a statistic, so it gets its own pattern rather than the trailing-number one).writePushJSONprefers it and falls back to the requested name when absent.destination tablefield; the string catalog golden is updated with exactly that one line.The CLI needs no knowledge of the per-ingestion-tables flag: the ingestor reports the label when the flag is off and the handle when it is on, so the reported value is correct in both modes and no naming rule is restated here.
Test plan
go test ./internal/submit/... ./internal/cli/...— all green. Five new tests, each mutation-proved:writePushJSONignores the reported tableTestWritePushJSON_ReportsPhysicalTableWhenIngestorNamesItTestSummaryParser_ExtractsDestinationTableBeyond the happy path, three cases guard the edges that would otherwise bite on the error path:
"table": "".ds_prefix and hyphens, so a sloppy regex could capture the wrong line and still look plausible. The test pins both.The parser test also asserts the surrounding counters still parse with the extra line present, so adding it cannot silently shift the banner's field extraction.
Note
Low Risk
Output and parsing changes only; backward-compatible fallback keeps legacy ingestor behavior unchanged when the new banner line is absent.
Overview
Fixes backend#2895 so ingest output names the table that was actually created, not just the operator’s
--namelabel.When the cluster uses per-ingestion tables, the real MySQL handle is a
ds_<hex>name while--nameis only catalog metadata.--output-jsonpreviously echoedspec["table"], so scripts could get a label thatdata deleteanddata listcannot resolve—often looking like a successful cleanup when data was still there.submit.Summarynow includesDestinationTable, parsed from the ingestor banner’sDestination table:line (paired with the ingestor change).writePushJSONprefers that value and falls back to the requested name when the banner omits it (older ingestors, failures before the banner, or nil summary on error paths). The human ingestion panel shows a destination table field when present.Tests cover JSON emission, banner parsing (including legacy banners and ingestor ID vs handle), and the string golden catalog picks up the new UI label.
Reviewed by Cursor Bugbot for commit 67cc2a8. Bugbot is set up for automated code reviews on this repo. Configure here.
Part of tracebloc/backend#2895 — NOT
Closes: this is one half of the fix. The ingestor/jobs-manager half must report the destination table and the CLI half must surface it, and #2895's acceptance is end-to-end — the reportedtablepassed straight totracebloc data deleteon a fleet withperIngestionTableson. Neither PR alone satisfies that, so neither should close the ticket.