From 4452487236aac37c76ec708dfe41146e9545557f Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 09:23:38 +0300 Subject: [PATCH 01/10] ddl_strategy: -singleton scope is now migration context Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemamanager/tablet_executor.go | 4 ++- go/vt/vttablet/onlineddl/executor.go | 36 ++++++++++++++++---------- go/vt/vttablet/onlineddl/schema.go | 6 +++-- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/go/vt/schemamanager/tablet_executor.go b/go/vt/schemamanager/tablet_executor.go index be7354de8c9..a6419bb4c0a 100644 --- a/go/vt/schemamanager/tablet_executor.go +++ b/go/vt/schemamanager/tablet_executor.go @@ -250,7 +250,9 @@ func (exec *TabletExecutor) executeSQL(ctx context.Context, sql string, execResu for _, onlineDDL := range onlineDDLs { if exec.ddlStrategySetting.IsSkipTopo() { exec.executeOnAllTablets(ctx, execResult, onlineDDL.SQL, true) - exec.wr.Logger().Printf("%s\n", onlineDDL.UUID) + if len(execResult.SuccessShards) > 0 { + exec.wr.Logger().Printf("%s\n", onlineDDL.UUID) + } } else { exec.executeOnlineDDL(ctx, execResult, onlineDDL) } diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 5e7d806b3a6..a6289d67d8c 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -1167,16 +1167,17 @@ func (e *Executor) readMigration(ctx context.Context, uuid string) (onlineDDL *s return nil, nil, ErrMigrationNotFound } onlineDDL = &schema.OnlineDDL{ - Keyspace: row["keyspace"].ToString(), - Table: row["mysql_table"].ToString(), - Schema: row["mysql_schema"].ToString(), - SQL: row["migration_statement"].ToString(), - UUID: row["migration_uuid"].ToString(), - Strategy: schema.DDLStrategy(row["strategy"].ToString()), - Options: row["options"].ToString(), - Status: schema.OnlineDDLStatus(row["migration_status"].ToString()), - Retries: row.AsInt64("retries", 0), - TabletAlias: row["tablet"].ToString(), + Keyspace: row["keyspace"].ToString(), + Table: row["mysql_table"].ToString(), + Schema: row["mysql_schema"].ToString(), + SQL: row["migration_statement"].ToString(), + UUID: row["migration_uuid"].ToString(), + Strategy: schema.DDLStrategy(row["strategy"].ToString()), + Options: row["options"].ToString(), + Status: schema.OnlineDDLStatus(row["migration_status"].ToString()), + Retries: row.AsInt64("retries", 0), + TabletAlias: row["tablet"].ToString(), + RequestContext: row["migration_context"].ToString(), } return onlineDDL, row, nil } @@ -2544,15 +2545,22 @@ func (e *Executor) SubmitMigration( } if onlineDDL.StrategySetting().IsSingleton() { + // We will reject this migration if there's any pending migration within a different context e.migrationMutex.Lock() defer e.migrationMutex.Unlock() - uuids, err := e.readPendingMigrationsUUIDs(ctx) + pendingUUIDs, err := e.readPendingMigrationsUUIDs(ctx) if err != nil { - return result, err + return nil, err } - if len(uuids) > 0 { - return result, fmt.Errorf("singleton migration rejected: found pending migrations [%s]", strings.Join(uuids, ", ")) + for _, pendingUUID := range pendingUUIDs { + pendingOnlineDDL, _, err := e.readMigration(ctx, pendingUUID) + if err != nil { + return nil, err + } + if pendingOnlineDDL.RequestContext != onlineDDL.RequestContext { + return nil, fmt.Errorf("singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.RequestContext) + } } } diff --git a/go/vt/vttablet/onlineddl/schema.go b/go/vt/vttablet/onlineddl/schema.go index 90adc05cd76..05717e10c94 100644 --- a/go/vt/vttablet/onlineddl/schema.go +++ b/go/vt/vttablet/onlineddl/schema.go @@ -248,7 +248,8 @@ const ( retries, ddl_action, artifacts, - tablet + tablet, + migration_context FROM _vt.schema_migrations WHERE migration_uuid=%a @@ -273,7 +274,8 @@ const ( retries, ddl_action, artifacts, - tablet + tablet, + migration_context FROM _vt.schema_migrations WHERE migration_status='ready' From 93a665f430df7fdd840ee96fc78158fb63353ec8 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 12:50:01 +0300 Subject: [PATCH 02/10] introducing -singleton-context Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schema/ddl_strategy.go | 15 +++++++++++---- go/vt/vttablet/onlineddl/executor.go | 24 ++++++++++++++++-------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/go/vt/schema/ddl_strategy.go b/go/vt/schema/ddl_strategy.go index 0b2fb8fada0..a05d5eabc52 100644 --- a/go/vt/schema/ddl_strategy.go +++ b/go/vt/schema/ddl_strategy.go @@ -28,9 +28,10 @@ var ( ) const ( - declarativeFlag = "declarative" - skipTopoFlag = "skip-topo" - singletonFlag = "singleton" + declarativeFlag = "declarative" + skipTopoFlag = "skip-topo" + singletonFlag = "singleton" + singletonContextFlag = "singleton-context" ) // DDLStrategy suggests how an ALTER TABLE should run (e.g. "direct", "online", "gh-ost" or "pt-osc") @@ -123,6 +124,11 @@ func (setting *DDLStrategySetting) IsSingleton() bool { return setting.hasFlag(singletonFlag) } +// IsSingletonContext checks if strategy options include -singleton-context +func (setting *DDLStrategySetting) IsSingletonContext() bool { + return setting.hasFlag(singletonContextFlag) +} + // RuntimeOptions returns the options used as runtime flags for given strategy, removing any internal hint options func (setting *DDLStrategySetting) RuntimeOptions() []string { opts, _ := shlex.Split(setting.Options) @@ -132,6 +138,7 @@ func (setting *DDLStrategySetting) RuntimeOptions() []string { case isFlag(opt, declarativeFlag): case isFlag(opt, skipTopoFlag): case isFlag(opt, singletonFlag): + case isFlag(opt, singletonContextFlag): default: validOpts = append(validOpts, opt) } @@ -142,7 +149,7 @@ func (setting *DDLStrategySetting) RuntimeOptions() []string { // IsSkipTopo suggests that DDL should apply to tables bypassing global topo request func (setting *DDLStrategySetting) IsSkipTopo() bool { switch { - case setting.IsSingleton(): + case setting.IsSingleton(), setting.IsSingletonContext(): return true case setting.hasFlag(skipTopoFlag): return true diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index a6289d67d8c..422e9767800 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2544,8 +2544,7 @@ func (e *Executor) SubmitMigration( return nil, err } - if onlineDDL.StrategySetting().IsSingleton() { - // We will reject this migration if there's any pending migration within a different context + if onlineDDL.StrategySetting().IsSingleton() || onlineDDL.StrategySetting().IsSingletonContext() { e.migrationMutex.Lock() defer e.migrationMutex.Unlock() @@ -2553,13 +2552,22 @@ func (e *Executor) SubmitMigration( if err != nil { return nil, err } - for _, pendingUUID := range pendingUUIDs { - pendingOnlineDDL, _, err := e.readMigration(ctx, pendingUUID) - if err != nil { - return nil, err + switch { + case onlineDDL.StrategySetting().IsSingleton(): + // We will reject this migration if there's any pending migration + if len(pendingUUIDs) > 0 { + return result, fmt.Errorf("singleton migration rejected: found pending migrations [%s]", strings.Join(pendingUUIDs, ", ")) } - if pendingOnlineDDL.RequestContext != onlineDDL.RequestContext { - return nil, fmt.Errorf("singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.RequestContext) + case onlineDDL.StrategySetting().IsSingletonContext(): + // We will reject this migration if there's any pending migration within a different context + for _, pendingUUID := range pendingUUIDs { + pendingOnlineDDL, _, err := e.readMigration(ctx, pendingUUID) + if err != nil { + return nil, err + } + if pendingOnlineDDL.RequestContext != onlineDDL.RequestContext { + return nil, fmt.Errorf("singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.RequestContext) + } } } } From b6bc204106a48dfd815d19d58d3276ce6cf8233b Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 13:17:16 +0300 Subject: [PATCH 03/10] endtoend tests for singleton-context Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../singleton/onlineddl_singleton_test.go | 72 +++++++++++++++---- 1 file changed, 60 insertions(+), 12 deletions(-) diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index 71db3ff4fe7..0d378f8d24c 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -39,13 +39,14 @@ var ( clusterInstance *cluster.LocalProcessCluster vtParams mysql.ConnParams - hostname = "localhost" - keyspaceName = "ks" - cell = "zone1" - schemaChangeDirectory = "" - tableName = `stress_test` - onlineDDLStrategy = "online -singleton" - createStatement = ` + hostname = "localhost" + keyspaceName = "ks" + cell = "zone1" + schemaChangeDirectory = "" + tableName = `stress_test` + onlineSingletonDDLStrategy = "online -singleton" + onlineSingletonContextDDLStrategy = "online -singleton-context" + createStatement = ` CREATE TABLE stress_test ( id bigint(20) not null, rand_val varchar(32) null default '', @@ -61,6 +62,11 @@ var ( alterTableThrottlingStatement = ` ALTER TABLE stress_test DROP COLUMN created_timestamp ` + multiAlterTableThrottlingStatement = ` + ALTER TABLE stress_test ENGINE=InnoDB, + ALTER TABLE stress_test ENGINE=InnoDB, + ALTER TABLE stress_test ENGINE=InnoDB + ` // A trivial statement which must succeed and does not change the schema alterTableTrivialStatement = ` ALTER TABLE stress_test ENGINE=InnoDB @@ -68,6 +74,7 @@ var ( dropStatement = ` DROP TABLE stress_test ` + multiDropStatements = `DROP TABLE IF EXISTS t1, DROP TABLE IF EXISTS t2, DROP TABLE IF EXISTS t3` ) func TestMain(m *testing.M) { @@ -145,7 +152,7 @@ func TestSchemaChange(t *testing.T) { // CREATE t.Run("CREATE TABLE", func(t *testing.T) { // The table does not exist - uuid := testOnlineDDLStatement(t, createStatement, onlineDDLStrategy, "vtgate", "", "", false) + uuid := testOnlineDDLStatement(t, createStatement, onlineSingletonDDLStrategy, "vtgate", "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, true) @@ -202,7 +209,7 @@ func TestSchemaChange(t *testing.T) { }) t.Run("successful online alter, vtgate", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, onlineDDLStrategy, "vtgate", "hint_col", "", false) + uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, onlineSingletonDDLStrategy, "vtgate", "hint_col", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false) @@ -217,10 +224,51 @@ func TestSchemaChange(t *testing.T) { checkTable(t, tableName, true) }) + var throttledUUIDs []string + // singleton-context + t.Run("throttled migrations, singleton-context", func(t *testing.T) { + uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "", false) + throttledUUIDs = strings.Split(uuidList, "\n") + assert.Equal(t, 3, len(throttledUUIDs)) + for _, uuid := range throttledUUIDs { + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning) + } + }) + t.Run("failed migrations, singleton-context", func(t *testing.T) { + uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "", false) + throttledUUIDs = strings.Split(uuidList, "\n") + assert.Equal(t, 3, len(throttledUUIDs)) + for _, uuid := range throttledUUIDs { + uuid = strings.TrimSpace(uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed) + } + }) + t.Run("terminate throttled migrations", func(t *testing.T) { + for _, uuid := range throttledUUIDs { + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning) + onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, true) + } + time.Sleep(2 * time.Second) + for _, uuid := range throttledUUIDs { + uuid = strings.TrimSpace(uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed) + } + }) + + t.Run("successful multiple statement, singleton-context, vtctl", func(t *testing.T) { + uuidList := testOnlineDDLStatement(t, multiDropStatements, onlineSingletonContextDDLStrategy, "vtctl", "", "", false) + uuidSlice := strings.Split(uuidList, "\n") + assert.Equal(t, 3, len(uuidSlice)) + for _, uuid := range uuidSlice { + uuid = strings.TrimSpace(uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + } + }) + //DROP t.Run("online DROP TABLE", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, dropStatement, onlineDDLStrategy, "vtgate", "", "", false) + uuid := testOnlineDDLStatement(t, dropStatement, onlineSingletonDDLStrategy, "vtgate", "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, false) @@ -283,7 +331,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string, expectError string, skipWait bool) (uuid string) { revertQuery := fmt.Sprintf("revert vitess_migration '%s'", revertUUID) if executeStrategy == "vtgate" { - result := onlineddl.VtgateExecDDL(t, &vtParams, onlineDDLStrategy, revertQuery, expectError) + result := onlineddl.VtgateExecDDL(t, &vtParams, onlineSingletonDDLStrategy, revertQuery, expectError) if result != nil { row := result.Named().Row() if row != nil { @@ -291,7 +339,7 @@ func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string } } } else { - output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: onlineDDLStrategy, SkipPreflight: true}) + output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: onlineSingletonDDLStrategy, SkipPreflight: true}) if expectError == "" { assert.NoError(t, err) uuid = output From 6ac4319536a68490b2b663b9e6cfbf5ac206ef03 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 13:20:04 +0300 Subject: [PATCH 04/10] shorter sleep in 0singleton tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../endtoend/onlineddl/singleton/onlineddl_singleton_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index 0d378f8d24c..1d06da82d5d 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -318,7 +318,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str fmt.Printf("<%s>\n", uuid) if !strategySetting.Strategy.IsDirect() && !skipWait { - time.Sleep(time.Second * 20) + time.Sleep(time.Second * 10) } if expectError == "" && expectHint != "" { From a6d0f6174e437dac4e242be88ce8ef90891e951b Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 13:49:21 +0300 Subject: [PATCH 05/10] fixing multi statement delimiter Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../onlineddl/singleton/onlineddl_singleton_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index 1d06da82d5d..d2f42fcff44 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -63,9 +63,9 @@ var ( ALTER TABLE stress_test DROP COLUMN created_timestamp ` multiAlterTableThrottlingStatement = ` - ALTER TABLE stress_test ENGINE=InnoDB, - ALTER TABLE stress_test ENGINE=InnoDB, - ALTER TABLE stress_test ENGINE=InnoDB + ALTER TABLE stress_test ENGINE=InnoDB; + ALTER TABLE stress_test ENGINE=InnoDB; + ALTER TABLE stress_test ENGINE=InnoDB; ` // A trivial statement which must succeed and does not change the schema alterTableTrivialStatement = ` @@ -74,7 +74,7 @@ var ( dropStatement = ` DROP TABLE stress_test ` - multiDropStatements = `DROP TABLE IF EXISTS t1, DROP TABLE IF EXISTS t2, DROP TABLE IF EXISTS t3` + multiDropStatements = `DROP TABLE IF EXISTS t1; DROP TABLE IF EXISTS t2; DROP TABLE IF EXISTS t3;` ) func TestMain(m *testing.M) { @@ -318,7 +318,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str fmt.Printf("<%s>\n", uuid) if !strategySetting.Strategy.IsDirect() && !skipWait { - time.Sleep(time.Second * 10) + time.Sleep(time.Second * 20) } if expectError == "" && expectHint != "" { From 9a9dda67c8a21a0ae088e7e9b61a77dec878af57 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 14:03:14 +0300 Subject: [PATCH 06/10] test: accept multiple statuses Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../onlineddl/singleton/onlineddl_singleton_test.go | 4 ++-- go/test/endtoend/onlineddl/vtgate_util.go | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index d2f42fcff44..78ae185f891 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -231,7 +231,7 @@ func TestSchemaChange(t *testing.T) { throttledUUIDs = strings.Split(uuidList, "\n") assert.Equal(t, 3, len(throttledUUIDs)) for _, uuid := range throttledUUIDs { - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning, schema.OnlineDDLStatusQueued) } }) t.Run("failed migrations, singleton-context", func(t *testing.T) { @@ -251,7 +251,7 @@ func TestSchemaChange(t *testing.T) { time.Sleep(2 * time.Second) for _, uuid := range throttledUUIDs { uuid = strings.TrimSpace(uuid) - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed, schema.OnlineDDLStatusCancelled) } }) diff --git a/go/test/endtoend/onlineddl/vtgate_util.go b/go/test/endtoend/onlineddl/vtgate_util.go index 833f155ec8f..0c1f3db23e6 100644 --- a/go/test/endtoend/onlineddl/vtgate_util.go +++ b/go/test/endtoend/onlineddl/vtgate_util.go @@ -107,7 +107,7 @@ func CheckCancelAllMigrations(t *testing.T, vtParams *mysql.ConnParams, expectCo } // CheckMigrationStatus verifies that the migration indicated by given UUID has the given expected status -func CheckMigrationStatus(t *testing.T, vtParams *mysql.ConnParams, shards []cluster.Shard, uuid string, expectStatus schema.OnlineDDLStatus) { +func CheckMigrationStatus(t *testing.T, vtParams *mysql.ConnParams, shards []cluster.Shard, uuid string, expectStatuses ...schema.OnlineDDLStatus) { showQuery := fmt.Sprintf("show vitess_migrations like '%s'", uuid) r := VtgateExecQuery(t, vtParams, showQuery, "") fmt.Printf("# output for `%s`:\n", showQuery) @@ -115,8 +115,14 @@ func CheckMigrationStatus(t *testing.T, vtParams *mysql.ConnParams, shards []clu count := 0 for _, row := range r.Named().Rows { - if row["migration_uuid"].ToString() == uuid && row["migration_status"].ToString() == string(expectStatus) { - count++ + if row["migration_uuid"].ToString() != uuid { + continue + } + for _, expectStatus := range expectStatuses { + if row["migration_status"].ToString() == string(expectStatus) { + count++ + break + } } } assert.Equal(t, len(shards), count) From 6aa8f80573066cc370b1f2fe4eafbc13240b585d Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 14:13:14 +0300 Subject: [PATCH 07/10] expect reject Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../onlineddl/singleton/onlineddl_singleton_test.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index 78ae185f891..3507164af95 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -235,13 +235,7 @@ func TestSchemaChange(t *testing.T) { } }) t.Run("failed migrations, singleton-context", func(t *testing.T) { - uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "", false) - throttledUUIDs = strings.Split(uuidList, "\n") - assert.Equal(t, 3, len(throttledUUIDs)) - for _, uuid := range throttledUUIDs { - uuid = strings.TrimSpace(uuid) - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed) - } + _ = testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "rejected", false) }) t.Run("terminate throttled migrations", func(t *testing.T) { for _, uuid := range throttledUUIDs { From 957ce5a1b52c0eb99c8f13179ea0a8aaf7292124 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 14:21:50 +0300 Subject: [PATCH 08/10] fix test condition Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../endtoend/onlineddl/singleton/onlineddl_singleton_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index 3507164af95..84a0ed91f38 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -239,7 +239,7 @@ func TestSchemaChange(t *testing.T) { }) t.Run("terminate throttled migrations", func(t *testing.T) { for _, uuid := range throttledUUIDs { - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning, schema.OnlineDDLStatusQueued) onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, true) } time.Sleep(2 * time.Second) From cb708039927e6a092b5fc5d40f132c3232f16e56 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Apr 2021 20:55:39 +0300 Subject: [PATCH 09/10] ensure schema exists Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/vttablet/onlineddl/executor.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 422e9767800..40cdde5357e 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2544,6 +2544,11 @@ func (e *Executor) SubmitMigration( return nil, err } + if err := e.initSchema(ctx); err != nil { + log.Error(err) + return nil, err + } + if onlineDDL.StrategySetting().IsSingleton() || onlineDDL.StrategySetting().IsSingletonContext() { e.migrationMutex.Lock() defer e.migrationMutex.Unlock() From 4d03f9b5e496000365f79572d4c9532cb53ca7ee Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 26 Apr 2021 08:41:25 +0300 Subject: [PATCH 10/10] fmt.Errorf -> vterrors.Errorf Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/vttablet/onlineddl/executor.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 40cdde5357e..ab3db8bb475 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2519,7 +2519,7 @@ func (e *Executor) SubmitMigration( onlineDDL, err := schema.OnlineDDLFromCommentedStatement(stmt) if err != nil { - return nil, fmt.Errorf("Error submitting migration %s: %v", sqlparser.String(stmt), err) + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Error submitting migration %s: %v", sqlparser.String(stmt), err) } _, actionStr, err := onlineDDL.GetActionStr() if err != nil { @@ -2561,7 +2561,7 @@ func (e *Executor) SubmitMigration( case onlineDDL.StrategySetting().IsSingleton(): // We will reject this migration if there's any pending migration if len(pendingUUIDs) > 0 { - return result, fmt.Errorf("singleton migration rejected: found pending migrations [%s]", strings.Join(pendingUUIDs, ", ")) + return result, vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "singleton migration rejected: found pending migrations [%s]", strings.Join(pendingUUIDs, ", ")) } case onlineDDL.StrategySetting().IsSingletonContext(): // We will reject this migration if there's any pending migration within a different context @@ -2571,7 +2571,7 @@ func (e *Executor) SubmitMigration( return nil, err } if pendingOnlineDDL.RequestContext != onlineDDL.RequestContext { - return nil, fmt.Errorf("singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.RequestContext) + return nil, vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.RequestContext) } } }