Address review: consistent receiver, migrator seam + test, localtest · github/gh-ost@7e43186 · GitHub
Skip to content

Commit 7e43186

Browse files
VandhanaSelvaprakash-atVandhanaSelvaprakash-at
committed
Address review: consistent receiver, migrator seam + test, localtest
- applier: name the AnalyzeGhostTable receiver `apl`, consistent with the rest of applier.go (staticcheck ST1016) — fixes the golangci-lint failure. - migrator: extract the pre-cut-over ANALYZE gating into analyzeGhostTableBeforeCutOver(analyze func() error), a narrow injectable seam (behavior unchanged), so the orchestration contract is unit-testable without a live applier or the process-exiting Log.Fatale path. - migrator test: TestAnalyzeGhostTableBeforeCutOver covers the flag gating, the happy path (ANALYZE runs once), and fail-closed (a failed ANALYZE propagates so cut-over aborts before replica-stop / cut-over locking). - localtest: analyze-ghost-table-before-cutover exercises the flag end-to-end against live MySQL with ongoing DML. Signed-off-by: Vandhana Selvaprakash <vandhana.selvaprakash@airtable.com>
1 parent 2425722 commit 7e43186

5 files changed

Lines changed: 110 additions & 23 deletions

File tree

‎go/logic/applier.go‎

Lines changed: 12 additions & 12 deletions

‎go/logic/migrator.go‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -854,6 +854,20 @@ func (mgtr *Migrator) handleCutOverResult(cutOverError error) (err error) {
854854
return nil
855855
}
856856

857+
// analyzeGhostTableBeforeCutOver runs the opt-in pre-cut-over ANALYZE TABLE via the
858+
// injected analyze operation, gated on --analyze-ghost-table-before-cutover. It
859+
// returns nil (proceed) when the flag is off, and otherwise returns whatever analyze
860+
// returns; the caller must treat a non-nil error as fatal and abort cut-over before
861+
// any source lock, replica stop, or cut-over retry. analyze is a parameter so the
862+
// gating and fail-closed contract is testable without a live applier (whose ANALYZE
863+
// requires a real MySQL) or the process-exiting Log.Fatale path.
864+
func (mgtr *Migrator) analyzeGhostTableBeforeCutOver(analyze func() error) error {
865+
if !mgtr.migrationContext.AnalyzeGhostTableBeforeCutOver {
866+
return nil
867+
}
868+
return analyze()
869+
}
870+
857871
// cutOver performs the final step of migration, based on migration
858872
// type (on replica? atomic? safe?)
859873
func (mgtr *Migrator) cutOver() (err error) {
@@ -904,17 +918,16 @@ func (mgtr *Migrator) cutOver() (err error) {
904918
mgtr.migrationContext.MarkPointOfInterest()
905919
mgtr.migrationContext.Log.Debugf("checking for cut-over postpone: complete")
906920

907-
if mgtr.migrationContext.AnalyzeGhostTableBeforeCutOver {
908-
// Force a synchronous ANALYZE on the ghost table here — after the postpone gate
909-
// releases, before atomicCutOver() takes the source write lock, and before
910-
// --test-on-replica stops replication (so a failure cannot strand a stopped
911-
// replica). A failure must be fatal, not retried: a plain `return err` re-runs
912-
// cutOver() — and the ANALYZE — up to --default-retries, and a PanicAbort send
913-
// races the retrier. Log.Fatale exits synchronously without ever locking the
914-
// source.
915-
if err := mgtr.applier.AnalyzeGhostTable(); err != nil {
916-
return mgtr.migrationContext.Log.Fatale(err)
917-
}
921+
// Force a synchronous ANALYZE on the ghost table here — after the postpone gate
922+
// releases, before atomicCutOver() takes the source write lock, and before
923+
// --test-on-replica stops replication (so a failure cannot strand a stopped
924+
// replica). A failure must be fatal, not retried: a plain `return err` re-runs
925+
// cutOver() — and the ANALYZE — up to --default-retries, and a PanicAbort send
926+
// races the retrier. Log.Fatale exits synchronously without ever locking the
927+
// source. Gating and the injectable analyze op live in
928+
// analyzeGhostTableBeforeCutOver so this ordering contract is unit-testable.
929+
if err := mgtr.analyzeGhostTableBeforeCutOver(mgtr.applier.AnalyzeGhostTable); err != nil {
930+
return mgtr.migrationContext.Log.Fatale(err)
918931
}
919932

920933
if mgtr.migrationContext.TestOnReplica {

‎go/logic/migrator_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,57 @@ func TestCutOverOperationWithMetricsAbort(t *testing.T) {
496496
assert.Equal(t, [][]string{{"outcome:" + metrics.CutOverOutcomeAbort}}, spy.histogramTags)
497497
}
498498

499+
// TestAnalyzeGhostTableBeforeCutOver covers the cut-over orchestration contract for
500+
// the opt-in pre-cut-over ANALYZE: the --analyze-ghost-table-before-cutover flag
501+
// gates the call, and a failed ANALYZE surfaces as an error so cutOver() aborts
502+
// (via Log.Fatale) before it reaches replica-stop or the cut-over locking/retry
503+
// switch. The applier's ANALYZE execution and result parsing are covered separately
504+
// by ApplierTestSuite.TestAnalyzeGhostTable against a real MySQL.
505+
func TestAnalyzeGhostTableBeforeCutOver(t *testing.T) {
506+
t.Run("flag off: ANALYZE is not invoked, cut-over proceeds", func(t *testing.T) {
507+
migrator := NewMigrator(base.NewMigrationContext(), "test")
508+
migrator.migrationContext.AnalyzeGhostTableBeforeCutOver = false
509+
510+
invoked := false
511+
err := migrator.analyzeGhostTableBeforeCutOver(func() error {
512+
invoked = true
513+
return nil
514+
})
515+
516+
require.NoError(t, err)
517+
assert.False(t, invoked, "ANALYZE must not run when the flag is off")
518+
})
519+
520+
t.Run("flag on, ANALYZE succeeds: invoked once, cut-over proceeds", func(t *testing.T) {
521+
migrator := NewMigrator(base.NewMigrationContext(), "test")
522+
migrator.migrationContext.AnalyzeGhostTableBeforeCutOver = true
523+
524+
calls := 0
525+
err := migrator.analyzeGhostTableBeforeCutOver(func() error {
526+
calls++
527+
return nil
528+
})
529+
530+
require.NoError(t, err)
531+
assert.Equal(t, 1, calls)
532+
})
533+
534+
t.Run("flag on, ANALYZE fails: error propagates so cut-over aborts fail-closed", func(t *testing.T) {
535+
migrator := NewMigrator(base.NewMigrationContext(), "test")
536+
migrator.migrationContext.AnalyzeGhostTableBeforeCutOver = true
537+
538+
analyzeErr := errors.New("ANALYZE TABLE on ghost failed; refusing cut-over")
539+
calls := 0
540+
err := migrator.analyzeGhostTableBeforeCutOver(func() error {
541+
calls++
542+
return analyzeErr
543+
})
544+
545+
require.ErrorIs(t, err, analyzeErr)
546+
assert.Equal(t, 1, calls, "a failed ANALYZE must not be retried inside the seam")
547+
})
548+
}
549+
499550
func TestReportStatusEmitsProgressGaugesEveryTick(t *testing.T) {
500551
spy := &progressGaugeSpy{}
501552
ctx := base.NewMigrationContext()

‎localtests/analyze-ghost-table-before-cutover/create.sql‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
drop table if exists gh_ost_test;
2+
create table gh_ost_test (
3+
id int auto_increment,
4+
i int not null,
5+
color varchar(32),
6+
primary key(id)
7+
) auto_increment=1;
8+
9+
drop event if exists gh_ost_test;
10+
delimiter ;;
11+
create event gh_ost_test
12+
on schedule every 1 second
13+
starts current_timestamp
14+
ends current_timestamp + interval 60 second
15+
on completion not preserve
16+
enable
17+
do
18+
begin
19+
insert into gh_ost_test values (null, 11, 'red');
20+
insert into gh_ost_test values (null, 13, 'green');
21+
update gh_ost_test set color = 'gray' where i = 11;
22+
end ;;

‎localtests/analyze-ghost-table-before-cutover/extra_args‎

Lines changed: 1 addition & 0 deletions

0 commit comments

Comments
 (0)