Skip to content

copier: expose remaining copy time as a structured ETA#985

Merged
morgo merged 6 commits into
mainfrom
aparajon/progress-eta-seconds
Jun 25, 2026
Merged

copier: expose remaining copy time as a structured ETA#985
morgo merged 6 commits into
mainfrom
aparajon/progress-eta-seconds

Conversation

@aparajon

@aparajon aparajon commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Progress carried the copy ETA only inside the human-readable Summary string, so programmatic consumers (e.g. a control plane rendering per-table progress) had to parse log output to recover it — and couldn't tell "still measuring" from a real estimate.

This adds a structured ETA to the status API:

  • status.ETA — a small value bundling State and Duration:
    • State: measuring (no copy rate yet, the TBD window), ready (Duration is valid), due (copy essentially complete, the DUE case), or none (not in row copy). Lets a consumer render "calculating…" during the initial window instead of a misleading 0.
    • Duration (time.Duration): the estimated remaining row-copy time, valid only when State == ready.
  • status.Progress.ETA — the structured estimate.
  • Copier.GetETAState() status.ETA — computes the state and duration in one read so they stay consistent.

A shared etaEstimate helper backs GetETA and GetETAState, removing the duplicated estimate math the buffered and unbuffered copiers each carried and keeping the string and structured forms in sync.

@aparajon aparajon force-pushed the aparajon/progress-eta-seconds branch 2 times, most recently from ff01bb3 to 30eacd0 Compare June 23, 2026 16:41
Progress only carried the ETA inside the human-readable Summary string, so
programmatic consumers (e.g. a control plane rendering per-table progress) had
to parse log output to recover it, and could not tell "still measuring" from a
real estimate.

Add Copier.GetETASeconds and Copier.GetETAState, and surface them as
status.Progress.ETASeconds and status.Progress.ETAState during row copy.
ETAState distinguishes measuring (no copy rate yet — the "TBD" case), ready
(ETASeconds is valid), and due (copy essentially complete — "DUE"), so callers
can show "calculating" instead of a misleading 0. A shared etaEstimate helper
backs GetETA, GetETASeconds, and GetETAState, removing the duplicated estimate
math across the buffered and unbuffered copiers.
@aparajon aparajon force-pushed the aparajon/progress-eta-seconds branch from 30eacd0 to a74c9f7 Compare June 23, 2026 16:56

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to merge in whats in #879 as well? These changes all serve the same purpose essentially.

Comment thread pkg/copier/copier.go Outdated
aparajon added 3 commits June 23, 2026 18:02
Address review feedback: prefer time.Duration over an int64 seconds count for
the structured ETA. GetETAState now returns (status.ETAState, time.Duration) and
status.Progress.ETA is a time.Duration, dropping the seconds conversion so the
value stays idiomatic end to end.
The ETA value and its availability are one concept, so carry them in one
status.ETA{State, Duration} value instead of two parallel Progress fields.
GetETAState returns the bundle directly, so the state and duration are read
together and stay consistent.
@morgo morgo marked this pull request as ready for review June 23, 2026 22:25
@morgo morgo requested a review from Copilot June 23, 2026 22:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends Spirit’s status/progress surface to expose row-copy ETA as a structured value (state + duration), instead of only embedding it in the human-readable Summary string. This enables programmatic consumers to distinguish “still measuring” vs “ready” vs “due” without parsing logs.

Changes:

  • Introduces status.ETAState and status.ETA, and adds ETA status.ETA to status.Progress.
  • Adds Copier.GetETAState() status.ETA and centralizes ETA math in a shared etaEstimate helper used by both buffered and unbuffered copiers.
  • Updates migration progress reporting and tests to populate/assert the structured ETA, and adds unit coverage for the shared ETA estimator.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/status/progress.go Adds structured ETA types (ETAState, ETA) and exposes ETA on status.Progress.
pkg/migration/runner.go Populates the new structured Progress.ETA during CopyRows.
pkg/migration/binlog_test.go Updates expected status.Progress values to include structured ETA state.
pkg/copier/copier.go Adds GetETAState() to the Copier interface and introduces shared etaEstimate.
pkg/copier/buffered.go Reuses etaEstimate for text ETA and adds GetETAState() implementation.
pkg/copier/unbuffered.go Reuses etaEstimate for text ETA and adds GetETAState() implementation.
pkg/copier/copier_eta_test.go Adds unit tests validating etaEstimate state/duration behavior.
Comments suppressed due to low confidence (1)

pkg/migration/runner.go:1026

  • Progress() builds Summary using r.copier.GetETA() and then separately reads r.copier.GetETAState(); since both values are recomputed from mutable copy stats, the human Summary and structured ETA can disagree at boundaries (e.g. switching from measuring→ready/due). Consider adding a single copier snapshot API (e.g. GetETASnapshot() (string, status.ETA)) or otherwise deriving Summary’s ETA text from the structured ETA so both representations are guaranteed consistent. Also note move/datasync runners include ETA text in Summary but currently would leave Progress.ETA at its zero value, which may surprise programmatic consumers of the shared status.Progress type.
	}
	// Single-table mode: the checkpoint table name is derived from the table
	// name and same-table concurrency is already excluded by the metadata

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/copier/copier_eta_test.go Outdated
@morgo morgo merged commit ce39327 into main Jun 25, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants