From 90899367ddd2aebe128f6db73aaa36b70f81ffe7 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sun, 6 Sep 2026 17:37:56 -0400 Subject: [PATCH] refactor: render Spirit progress from structured fields, not a parsed string Spirit's status.Progress already carries the row counts, the checksum counters, the throttle status, and the ETA as typed fields, and the engine wrapper consumes them that way. The CLI still kept a regex that parsed a Spirit-shaped "rows/total pct% phase" line back into numbers, and the engine synthesized exactly such a line so the CLI could parse it again. Nothing on the wire ever populated that field, so the parser only ran against preview fixtures. Delete the parser and both call sites, stop synthesizing the line, and rewrite the parts of docs/spirit_progress.md that still described the ETA as regex-parsed out of Summary. ProgressDetail stays a free-text note for a human reader. Co-Authored-By: Claude Fable 5 --- docs/spirit_progress.md | 43 ++++++++------- pkg/cmd/commands/apply_log_test.go | 3 +- pkg/cmd/commands/preview_log.go | 27 +++++---- pkg/cmd/internal/templates/progress.go | 32 +---------- pkg/cmd/internal/templates/progress_parse.go | 55 ------------------- .../internal/templates/progress_parse_test.go | 14 ++--- .../templates/progress_states_test.go | 54 ++++++------------ pkg/engine/engine.go | 2 +- pkg/engine/spirit/drain_outcome_test.go | 21 +++---- pkg/engine/spirit/spirit.go | 8 +-- pkg/engine/spirit/spirit_test.go | 3 - 11 files changed, 74 insertions(+), 188 deletions(-) diff --git a/docs/spirit_progress.md b/docs/spirit_progress.md index dd4a5e9f3..d0d861a29 100644 --- a/docs/spirit_progress.md +++ b/docs/spirit_progress.md @@ -48,13 +48,15 @@ key to debugging stale-progress issues. | Field | Type | Notes | |-----------------|-------------------|-------| | `CurrentState` | `status.State` | Atomic int32 enum: `Initial`, `CopyRows`, `WaitingOnSentinelTable`, `Checksum`, `CutOver`, `Close`, ... | -| `Summary` | `string` | `"71436/221193 32.30% copyRows ETA 5m 30s"` | +| `Summary` | `string` | Human-readable line for logs, e.g. `"71436/221193 32.30% copyRows ETA 5m 30s"`. Display-only: every value in it is also one of the structured fields below, and nothing in SchemaBot parses it. | | `Tables[]` | `[]TableProgress` | Per-table: `TableName`, `RowsCopied` (uint64), `RowsTotal` (uint64), `IsComplete` (bool) | +| `ETA` | `status.ETA` | `State` (`""` outside the copy, `measuring`, `ready`, `due`) and `Duration` (remaining copy time, meaningful only when `State == ETAReady`). | +| `Checksum` | `status.ChecksumProgress` | `RowsChecked` / `RowsTotal` for the verify phase; zero outside it. | | `Resume` | `bool` | True only after the runner successfully resumed from its durable checkpoint; a fresh start (or an abandoned resume attempt) reports false. | | `Throttle` | `status.ThrottleStatus` | `Throttled` (bool), `Reason` (display-only string, `" "`), `Utilization` (float64, 0 means unknown — never render it as idle). | Key details: -- **ETA is embedded in `Summary`**, not a separate field. Downstream layers parse it out with a regex. +- **The ETA is a structured field.** `ETA.State` says whether an estimate exists yet — `measuring` during the initial window before a copy rate is known, `due` once the copy is essentially finished — so a consumer can show "calculating" instead of a misleading zero. `ETA.Duration` is only meaningful when the state is `ready`. If Spirit reports something SchemaBot needs and it is only in `Summary`, the fix is a new field on `status.Progress` upstream, not a parser here. - **`IsComplete`** comes from the chunker's in-memory `finalChunkSent` flag, NOT from the checkpoint table. This means `IsComplete` is lost on crash — it only exists while the runner is alive. - **`RowsCopied` can exceed `RowsTotal`** when the initial MySQL estimate is low. Downstream renderers treat this as an active estimate-exceeded state rather than a percentage above 100%. - **`Checksum` progress (`prog.Checksum.RowsChecked` / `RowsTotal`) is populated only while @@ -111,7 +113,9 @@ Two properties matter for display: - Each table's `State` is the raw Spirit phase string (`copyRows`, `applyChangeset`, ...). - Calculates `Progress` percent (clamped 0–100) and preserves raw `RowsCopied` so renderers can detect when the initial estimate was exceeded. - - Sets `ProgressDetail` = formatted summary like `"12345/50000 24% copyRows"`. + - Surfaces the runner's single `ETA` as `ETASeconds` on the tables still + copying, and only when `ETA.State` is `ready` — a still-measuring or + essentially-done estimate is not yet a number. - When `IsComplete` is true, reconciles `RowsTotal = RowsCopied` (the estimate was never a count; the copied total is ground truth once the copy finishes) and sets `Progress` to 100. The table keeps the runner phase while the @@ -133,6 +137,11 @@ Two properties matter for display: Key types: `engine.ProgressResult`, `engine.TableProgress` (`pkg/engine/engine.go`). +`engine.TableProgress.ProgressDetail` is a free-text note for a human, never a +data channel: Spirit's `Summary` line while the runner has no per-table progress +yet, or a marker that a statement ran as native DDL outside the runner. Nothing +downstream parses it, and the drive does not persist it. + When no runner exists (engine stopped, no active schema change), returns `StatePending` with message `"No active schema change"`. @@ -280,19 +289,12 @@ adds apply-level fields: `apply_id`, `database`, `environment`. The TUI polls the API every **2 seconds** via `tick()`. -`parseProgressResult()` converts the JSON response to internal types. For each table, -if `ProgressDetail` is non-empty, it runs `ParseSpiritProgress()` — a regex parser -in `pkg/cmd/internal/templates/progress.go` that extracts structured data from Spirit's summary string: - -``` -"71436/221193 32.30% copyRows ETA 5m 30s" - ↓ ↓ ↓ ↓ ↓ -RowsCopied RowsTotal Percent State ETA -``` - -The parsed values override the structured API fields (`RowsCopied`, `RowsTotal`, `Percent`) -because `ProgressDetail` comes directly from Spirit and is more current than the separately-polled -numeric fields. +`parseProgressResult()` converts the JSON response to internal types through +`templates.ParseProgressResponse()`, a field-for-field mapping of the structured +API fields (`RowsCopied`, `RowsTotal`, `PercentComplete`, `ETASeconds`, the +checksum counters, the throttle status). The renderer draws the bar, the rows +line, and the ETA from those fields alone — the same fields the PR comment +renders from, so the two surfaces always agree. ## TUI rendering reference @@ -446,9 +448,12 @@ The `⠋` is a Braille spinner (animated in the TUI, static here). applies with stale heartbeats and call `Tern.ResumeApply()`, which re-plans against the actual DB state to determine what still needs to be done. -5. **ETA is only available via `ProgressDetail` parsing.** Spirit embeds ETA in its summary string. - The engine layer doesn't extract it into a separate field — it flows through as `ProgressDetail` - and is parsed by the CLI with a regex. If the regex fails, no ETA is shown. +5. **The ETA is structured end to end.** Spirit reports `status.ETA{State, Duration}`; the + engine surfaces it as `ETASeconds` only when the state is `ready`; the drive persists it + on the task row; the API and CLI carry it as `eta_seconds` / `ETASeconds`; and both the CLI + and the PR comment render it through `ui.FormatETA`. No layer derives it from text, so a + missing ETA means Spirit had none to give (still measuring, or essentially done), not that + a parser failed. 6. **Estimate-exceeded display.** Spirit can report `RowsCopied > RowsTotal` when MySQL's initial estimate is low. SchemaBot preserves the raw copied count, clamps determinate percentages to 100, diff --git a/pkg/cmd/commands/apply_log_test.go b/pkg/cmd/commands/apply_log_test.go index c6e7269ec..a57233c00 100644 --- a/pkg/cmd/commands/apply_log_test.go +++ b/pkg/cmd/commands/apply_log_test.go @@ -215,7 +215,7 @@ func TestLogEmitter_EmitTableStateChange(t *testing.T) { } func TestLogEmitter_EmitProgressHeartbeat(t *testing.T) { - t.Run("structured ETA renders alongside a Spirit progress detail", func(t *testing.T) { + t.Run("structured ETA renders alongside the row counts", func(t *testing.T) { e := &logEmitter{applyID: "apply-test"} ts := &tableLogState{taskID: "task-orders-1"} tbl := &apitypes.TableProgressResponse{ @@ -224,7 +224,6 @@ func TestLogEmitter_EmitProgressHeartbeat(t *testing.T) { RowsCopied: 99450, RowsTotal: 221000, ETASeconds: 330, - ProgressDetail: "99450/221000 45.00% copyRows", } output := captureOutput(t, func() { diff --git a/pkg/cmd/commands/preview_log.go b/pkg/cmd/commands/preview_log.go index 9124228d8..c05a51b1d 100644 --- a/pkg/cmd/commands/preview_log.go +++ b/pkg/cmd/commands/preview_log.go @@ -102,22 +102,22 @@ func previewLogLarge() { // Heartbeats at 30s intervals heartbeats := []struct { - pct int32 - copied int64 - total int64 - eta string + pct int32 + copied int64 + total int64 + etaSeconds int64 }{ - {12, 26543, 221193, "99450/221193 12.00% copyRows ETA 4m 10s"}, - {25, 55298, 221193, "99450/221193 25.00% copyRows ETA 3m 30s"}, - {45, 99537, 221193, "99450/221193 45.00% copyRows ETA 2m 15s"}, - {68, 150412, 221193, "150412/221193 68.00% copyRows ETA 1m 10s"}, - {89, 196861, 221193, "196861/221193 89.00% copyRows ETA 25s"}, + {12, 26543, 221193, 250}, + {25, 55298, 221193, 210}, + {45, 99537, 221193, 135}, + {68, 150412, 221193, 70}, + {89, 196861, 221193, 25}, } for _, hb := range heartbeats { tbl.PercentComplete = hb.pct tbl.RowsCopied = hb.copied tbl.RowsTotal = hb.total - tbl.ProgressDetail = hb.eta + tbl.ETASeconds = hb.etaSeconds e.emitProgressHeartbeat(tbl, ts) } @@ -213,13 +213,13 @@ func previewLogMulti() { tblOrders.PercentComplete = 30 tblOrders.RowsCopied = 66357 tblOrders.RowsTotal = 221193 - tblOrders.ProgressDetail = "66357/221193 30.00% copyRows ETA 2m 30s" + tblOrders.ETASeconds = 150 e.emitProgressHeartbeat(tblOrders, tsOrders) tblOrders.PercentComplete = 65 tblOrders.RowsCopied = 143776 tblOrders.RowsTotal = 221193 - tblOrders.ProgressDetail = "143776/221193 65.00% copyRows ETA 1m 5s" + tblOrders.ETASeconds = 65 e.emitProgressHeartbeat(tblOrders, tsOrders) e.emitTableStateChange(tblOrders, state.Apply.Completed, tsOrders) @@ -245,7 +245,7 @@ func previewLogCutover() { tbl.PercentComplete = 50 tbl.RowsCopied = 110000 tbl.RowsTotal = 221193 - tbl.ProgressDetail = "110000/221193 50.00% copyRows ETA 2m 30s" + tbl.ETASeconds = 150 e.emitProgressHeartbeat(tbl, ts) // Waiting for cutover @@ -276,7 +276,6 @@ func previewLogDetailed() { RowsCopied: 148102, RowsTotal: 221193, ETASeconds: 45, - ProgressDetail: "148102/221193 67.00% copyRows ETA 45s", } e.emit(append(tableKVs("Table started", tbl, ts), diff --git a/pkg/cmd/internal/templates/progress.go b/pkg/cmd/internal/templates/progress.go index 88024438d..d682a235e 100644 --- a/pkg/cmd/internal/templates/progress.go +++ b/pkg/cmd/internal/templates/progress.go @@ -672,37 +672,9 @@ func FormatTableProgressWithActivity(t TableProgress, activityBar, activityLabel return b.String() } - // In-progress state - try to parse Spirit's progress detail + // In-progress state — rendered from the structured copy fields, which are + // the same source the PR comment renders from. switch { - case t.ProgressDetail != "": - if info := ParseSpiritProgress(t.ProgressDetail); info != nil { - if ui.EstimateExceeded(info.RowsCopied, info.RowsTotal) && info.State == "copyRows" { - b.WriteString(formatEstimateExceededTable(t, info.RowsCopied, activityBar, activityLabel)) - return b.String() - } - - // Parsed successfully - show emoji progress bar with structured data - displayPercent := ui.RowCopyDisplayPercent(info.Percent, info.RowsCopied) - bar := ui.ProgressBarRowCopy(displayPercent) - fmt.Fprintf(&b, indentTable+progressSymbol(t.ChangeType)+"%s: %s %s%s\n", t.TableName, bar, - ui.FormatRowCopyPercent(info.Percent, info.RowsCopied, info.RowsTotal), throttledSuffix(t)) - if t.DDL != "" { - b.WriteString(formatProgressDDLForDialect(t.Dialect, t.DDL)) - } - // Rows and ETA on the same line, rendered from the structured ETA - // so the CLI and PR comment show the same value via FormatETA. - writeStructuredRowsAndETA(&b, t) - if info.State != "" && info.State != "copyRows" { - fmt.Fprintf(&b, indentDetail+"Status: %s\n", info.State) - } - } else { - // Can't parse - show raw detail - fmt.Fprintf(&b, indentTable+progressSymbol(t.ChangeType)+"%s:\n", t.TableName) - if t.DDL != "" { - b.WriteString(formatProgressDDLForDialect(t.Dialect, t.DDL)) - } - fmt.Fprintf(&b, " %s\n", t.ProgressDetail) - } case t.RowsTotal > 0 && t.RowsCopied == 0: // Row total is known but the copy hasn't reported progress yet // (Vitess VReplication / Spirit ramp-up — can take a while on a large diff --git a/pkg/cmd/internal/templates/progress_parse.go b/pkg/cmd/internal/templates/progress_parse.go index 0cfcfa8b4..dda1347bc 100644 --- a/pkg/cmd/internal/templates/progress_parse.go +++ b/pkg/cmd/internal/templates/progress_parse.go @@ -1,10 +1,6 @@ package templates import ( - "math" - "regexp" - "strconv" - "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/ddl" "github.com/block/schemabot/pkg/schema" @@ -12,11 +8,6 @@ import ( "github.com/block/schemabot/pkg/ui" ) -// spiritProgressPattern matches the row-copy prefix of a Spirit progress -// string, e.g. "71436/221193 32.30% copyRows". The ETA is carried separately as -// a structured field, so it is not parsed out of this string. -var spiritProgressPattern = regexp.MustCompile(`(\d+)/(\d+)\s+([\d.]+)%\s+(\w+)`) - // ProgressData contains data for rendering schema change progress. type ProgressData struct { ApplyID string @@ -78,7 +69,6 @@ type TableProgress struct { Throttled bool ThrottleReason string IsInstant bool - ProgressDetail string // e.g., Spirit: "12.5% copyRows ETA 1h 30m" Shards []ShardProgress } @@ -105,39 +95,6 @@ type ShardCounts struct { Cancelled int } -// SpiritProgressInfo contains parsed Spirit progress information. -type SpiritProgressInfo struct { - RowsCopied int64 - RowsTotal int64 - Percent int - State string // "copyRows", "checksum", etc. -} - -// ParseSpiritProgress parses a Spirit progress string like "71436/221193 32.30% copyRows ETA TBD" -// Returns nil if the string cannot be parsed. -func ParseSpiritProgress(progress string) *SpiritProgressInfo { - if progress == "" { - return nil - } - - matches := spiritProgressPattern.FindStringSubmatch(progress) - if len(matches) < 5 { - return nil - } - - rowsCopied, _ := strconv.ParseInt(matches[1], 10, 64) - rowsTotal, _ := strconv.ParseInt(matches[2], 10, 64) - percentFloat, _ := strconv.ParseFloat(matches[3], 64) - state := matches[4] - - return &SpiritProgressInfo{ - RowsCopied: rowsCopied, - RowsTotal: rowsTotal, - Percent: int(math.Round(percentFloat)), - State: state, - } -} - // Display-only task states. These are not persisted apply states (see pkg/applystate) // but are used for per-table rendering in sequential mode. const ( @@ -198,18 +155,6 @@ func ParseProgressResponse(result *apitypes.ProgressResponse) ProgressData { Throttled: tbl.Throttled, ThrottleReason: tbl.ThrottleReason, IsInstant: tbl.IsInstant, - ProgressDetail: tbl.ProgressDetail, - } - // When a table carries an engine progress string, it is fresher than - // the stored copy fields, so prefer it and keep the percent, the rows - // line, and anything aggregated from them in agreement. The live - // progress API sends ProgressDetail empty (the drive loop does not - // persist it to the task record), so this override only takes effect - // for responses that populate the field, such as log preview fixtures. - if info := ParseSpiritProgress(tp.ProgressDetail); info != nil { - tp.PercentComplete = info.Percent - tp.RowsCopied = info.RowsCopied - tp.RowsTotal = info.RowsTotal } for _, sh := range tbl.Shards { pct := int(sh.PercentComplete) diff --git a/pkg/cmd/internal/templates/progress_parse_test.go b/pkg/cmd/internal/templates/progress_parse_test.go index 710812fdb..467948e80 100644 --- a/pkg/cmd/internal/templates/progress_parse_test.go +++ b/pkg/cmd/internal/templates/progress_parse_test.go @@ -181,20 +181,18 @@ func TestParseProgressResponseFiltersSpiritInternalTables(t *testing.T) { assert.Equal(t, "users", data.Tables[0].TableName) } -// Spirit's progress string is the freshest copy signal: when it parses, the -// structured percent and row counts follow it so every consumer renders the -// same numbers, and raw engine statuses normalize to canonical task states. -func TestParseProgressResponsePrefersSpiritProgressStringAndNormalizesStatus(t *testing.T) { +// The structured copy fields pass through untouched, and a raw engine phase +// string normalizes to its canonical task state. +func TestParseProgressResponseCarriesCopyFieldsAndNormalizesStatus(t *testing.T) { result := &apitypes.ProgressResponse{ State: state.Apply.Running, Tables: []*apitypes.TableProgressResponse{ { TableName: "users", Status: "copyRows", - RowsCopied: 100, - RowsTotal: 200, - PercentComplete: 50, - ProgressDetail: "71436/221193 32.30% copyRows ETA TBD", + RowsCopied: 71436, + RowsTotal: 221193, + PercentComplete: 32, }, }, } diff --git a/pkg/cmd/internal/templates/progress_states_test.go b/pkg/cmd/internal/templates/progress_states_test.go index 7ef7d461d..5f13955aa 100644 --- a/pkg/cmd/internal/templates/progress_states_test.go +++ b/pkg/cmd/internal/templates/progress_states_test.go @@ -867,10 +867,9 @@ func TestFormatTableProgress_SubPercentRowCopyShowsFraction(t *testing.T) { assert.NotContains(t, output, " 0%") } -// A Spirit row-copy reports its detail string and a structured ETA. The CLI -// renders the ETA from the structured field (the same source and FormatETA the -// PR comment uses), so the two surfaces show an identical "Rows … · ETA …" line -// even though the detail string itself no longer carries the ETA. +// A row copy renders its ETA from the structured field (the same source and +// FormatETA the PR comment uses), so the two surfaces show an identical +// "Rows … · ETA …" line. func TestFormatTableProgress_RowCopyShowsStructuredETA(t *testing.T) { tp := TableProgress{ TableName: "users", @@ -880,7 +879,6 @@ func TestFormatTableProgress_RowCopyShowsStructuredETA(t *testing.T) { RowsTotal: 100_000, PercentComplete: 45, ETASeconds: 340, - ProgressDetail: "45000/100000 45% copyRows", } output := FormatTableProgress(tp) @@ -915,38 +913,22 @@ func TestFormatTableProgress_FailedRetryableKeepsProgress(t *testing.T) { } func TestFormatTableProgress_EstimateExceeded(t *testing.T) { - t.Run("structured progress", func(t *testing.T) { - tp := TableProgress{ - TableName: "users", - ChangeType: "alter", - Status: state.Apply.Running, - RowsCopied: 145000, - RowsTotal: 100000, - PercentComplete: 145, - } - - output := FormatTableProgress(tp) - assert.Contains(t, output, ui.ProgressBarActivity()+" Finalizing copy") - assert.Contains(t, output, "Rows copied: 145,000 so far") - assert.Contains(t, output, ui.EstimateExceededTooltip) - assert.NotContains(t, output, "145%") - assert.NotContains(t, output, "100%") - assert.NotContains(t, output, "100,000 / 100,000") - }) - - t.Run("parsed Spirit progress", func(t *testing.T) { - tp := TableProgress{ - TableName: "users", - ChangeType: "alter", - Status: state.Apply.Running, - ProgressDetail: "145000/100000 100% copyRows ETA TBD", - } + tp := TableProgress{ + TableName: "users", + ChangeType: "alter", + Status: state.Apply.Running, + RowsCopied: 145000, + RowsTotal: 100000, + PercentComplete: 145, + } - output := FormatTableProgress(tp) - assert.Contains(t, output, ui.ProgressBarActivity()+" Finalizing copy") - assert.Contains(t, output, "Rows copied: 145,000 so far") - assert.NotContains(t, output, "100%") - }) + output := FormatTableProgress(tp) + assert.Contains(t, output, ui.ProgressBarActivity()+" Finalizing copy") + assert.Contains(t, output, "Rows copied: 145,000 so far") + assert.Contains(t, output, ui.EstimateExceededTooltip) + assert.NotContains(t, output, "145%") + assert.NotContains(t, output, "100%") + assert.NotContains(t, output, "100,000 / 100,000") } func TestFormatVSchemaStatus(t *testing.T) { diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 3d1d6d9ca..75b909b7d 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -738,7 +738,7 @@ type TableProgress struct { ThrottleReason string Shards []ShardProgress // Per-shard breakdown (for Vitess) IsInstant bool // True if using instant DDL - ProgressDetail string // Human-readable progress (e.g., Spirit: "12.5% copyRows ETA 1h 30m") + ProgressDetail string // Free-text note for a human reader; never parsed, never persisted DDL string // The DDL statement being applied StartedAt *time.Time // When execution actually began (from engine, e.g., SHOW VITESS_MIGRATIONS started_timestamp) CompletedAt *time.Time // When execution completed (from engine) diff --git a/pkg/engine/spirit/drain_outcome_test.go b/pkg/engine/spirit/drain_outcome_test.go index 9a127ca48..e3b51b545 100644 --- a/pkg/engine/spirit/drain_outcome_test.go +++ b/pkg/engine/spirit/drain_outcome_test.go @@ -97,7 +97,6 @@ func TestDrainedFailureKeepsLastObservedCopyPosition(t *testing.T) { RowsCopied: 42000, RowsTotal: 50000, Progress: 84, - ProgressDetail: "42000/50000 84% copyRows", ETASeconds: 12, Throttled: true, ThrottleReason: "replica lag", @@ -115,7 +114,6 @@ func TestDrainedFailureKeepsLastObservedCopyPosition(t *testing.T) { assert.Equal(t, int64(42000), tp.RowsCopied) assert.Equal(t, int64(50000), tp.RowsTotal) assert.Equal(t, 84, tp.Progress) - assert.Equal(t, "42000/50000 84% copyRows", tp.ProgressDetail) assert.Zero(t, tp.ETASeconds) assert.False(t, tp.Throttled) assert.Empty(t, tp.ThrottleReason) @@ -124,8 +122,7 @@ func TestDrainedFailureKeepsLastObservedCopyPosition(t *testing.T) { // A completed schema change copied everything, so the drained outcome reports // a full bar with the row total reconciled to the copied count — the count is // ground truth once the copy finished, and a full bar contradicted by its own -// rows line would misreport a change that landed. The mid-copy detail line is -// dropped: its stale percentage would contradict the completed state. +// rows line would misreport a change that landed. func TestDrainedCompletionReportsFullProgressWithLastCounters(t *testing.T) { eng := New(Config{}) rm := registerRunningSchemaChange(eng) @@ -135,14 +132,13 @@ func TestDrainedCompletionReportsFullProgressWithLastCounters(t *testing.T) { rm.ddls = []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255) NULL"} rm.tableNamespace = map[string]string{"users": "testdb"} rm.lastLiveTables = []engine.TableProgress{{ - Namespace: "testdb", - Table: "users", - DDL: "ALTER TABLE `users` ADD COLUMN `email` varchar(255) NULL", - State: "copyRows", - RowsCopied: 49000, - RowsTotal: 50000, - Progress: 98, - ProgressDetail: "49000/50000 98% copyRows", + Namespace: "testdb", + Table: "users", + DDL: "ALTER TABLE `users` ADD COLUMN `email` varchar(255) NULL", + State: "copyRows", + RowsCopied: 49000, + RowsTotal: 50000, + Progress: 98, }} eng.mu.Unlock() @@ -158,7 +154,6 @@ func TestDrainedCompletionReportsFullProgressWithLastCounters(t *testing.T) { assert.Equal(t, int64(49000), tp.RowsCopied) assert.Equal(t, int64(49000), tp.RowsTotal, "the estimated total reconciles to the copied count once the copy is complete") - assert.Empty(t, tp.ProgressDetail) } // Draining an engine that is not tracking any schema change changes nothing: diff --git a/pkg/engine/spirit/spirit.go b/pkg/engine/spirit/spirit.go index 21ccd8f74..041af96af 100644 --- a/pkg/engine/spirit/spirit.go +++ b/pkg/engine/spirit/spirit.go @@ -400,10 +400,8 @@ func newDrainedOutcome(rm *runningSchemaChange) *drainedOutcome { tp.Progress = 100 // A completed change copied everything, so the copied count is // ground truth: reconcile the estimated total to it the way the - // live path does, and drop the detail line whose mid-copy - // percentage would contradict the completed bar. + // live path does. tp.RowsTotal = tp.RowsCopied - tp.ProgressDetail = "" } tables = append(tables, tp) } @@ -972,10 +970,6 @@ func buildSpiritTableProgress(prog status.Progress, spiritState status.State, dd tp.Progress = min(int(float64(st.RowsCopied)/float64(st.RowsTotal)*100), 100) tp.ETASeconds = etaSeconds } - if tp.RowsTotal > 0 { - tp.ProgressDetail = fmt.Sprintf("%d/%d %d%% copyRows", - tp.RowsCopied, tp.RowsTotal, tp.Progress) - } // Spirit reports a single runner-wide checksum estimate (rows verified so // far / total to verify), populated only during the verify phase and zero // otherwise. Every table copy is complete by the time the verify phase diff --git a/pkg/engine/spirit/spirit_test.go b/pkg/engine/spirit/spirit_test.go index 096b311d2..227668ec9 100644 --- a/pkg/engine/spirit/spirit_test.go +++ b/pkg/engine/spirit/spirit_test.go @@ -129,7 +129,6 @@ func TestBuildSpiritTableProgress(t *testing.T) { assert.Equal(t, int64(45000), users.RowsCopied) assert.Equal(t, int64(100000), users.RowsTotal) assert.Equal(t, 45, users.Progress) - assert.Equal(t, "45000/100000 45% copyRows", users.ProgressDetail) assert.Equal(t, int64(90), users.ETASeconds) orders := got[1] @@ -147,7 +146,6 @@ func TestBuildSpiritTableProgress(t *testing.T) { require.Len(t, got, 1) assert.Equal(t, int64(0), got[0].ETASeconds, "no row total means no ETA") assert.Equal(t, 0, got[0].Progress) - assert.Empty(t, got[0].ProgressDetail) }) // A completed copy is a count, not an estimate: the reported total is @@ -167,7 +165,6 @@ func TestBuildSpiritTableProgress(t *testing.T) { assert.Equal(t, int64(3261100506), estimateHigh.RowsCopied) assert.Equal(t, int64(3261100506), estimateHigh.RowsTotal) assert.Equal(t, 100, estimateHigh.Progress) - assert.Equal(t, "3261100506/3261100506 100% copyRows", estimateHigh.ProgressDetail) estimateLow := got[1] assert.Equal(t, int64(1200), estimateLow.RowsCopied)