From 4470429c4191a64cf45dffa39728ac11dae2ec05 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 11 Jul 2021 15:26:34 +0300 Subject: [PATCH 1/3] -skip-topo is always 'true' Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schema/ddl_strategy.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/go/vt/schema/ddl_strategy.go b/go/vt/schema/ddl_strategy.go index dbc1f2d32b6..ad2b98e563c 100644 --- a/go/vt/schema/ddl_strategy.go +++ b/go/vt/schema/ddl_strategy.go @@ -155,13 +155,10 @@ 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(), setting.IsSingletonContext(): - return true - case setting.hasFlag(skipTopoFlag): - return true - } - return false + // Vitess 11 introduced the flag -skip-topo. starting Vitess 12 the flag is _always_ considered 'true'. + // Ideally the flag should be gone, but for backwards compatibility we allow users to still specify it + // (and we stil ignore the value, it's always set to true) + return true } // ToString returns a simple string representation of this instance From 48d14cd0fbc8e0ad78f57e3e309d9b2102d65a4e Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 11 Jul 2021 15:54:05 +0300 Subject: [PATCH 2/3] adapting tests to new 'always on' skip-topo Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schema/online_ddl.go | 26 ++++++++++++++++++++++++++ go/vt/schema/online_ddl_test.go | 22 +++++++++------------- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/go/vt/schema/online_ddl.go b/go/vt/schema/online_ddl.go index 81f0d506f2c..a67fe0cc268 100644 --- a/go/vt/schema/online_ddl.go +++ b/go/vt/schema/online_ddl.go @@ -372,6 +372,32 @@ func (onlineDDL *OnlineDDL) ToJSON() ([]byte, error) { return json.Marshal(onlineDDL) } +// sqlWithoutComments returns the SQL statement without comment directives. Useful for tests +func (onlineDDL *OnlineDDL) sqlWithoutComments() (sql string, err error) { + sql = onlineDDL.SQL + stmt, err := sqlparser.Parse(sql) + if err != nil { + // query validation and rebuilding + if _, err := legacyParseRevertUUID(sql); err == nil { + // This is a revert statement of the form "revert ". We allow this for now. Future work will + // make sure the statement is a valid, parseable "revert vitess_migration ''", but we must + // be backwards compatible for now. + return sql, nil + } + // otherwise the statement should have been parseable! + return "", err + } + + switch stmt := stmt.(type) { + case sqlparser.DDLStatement: + stmt.SetComments(nil) + case *sqlparser.RevertMigration: + stmt.SetComments(nil) + } + sql = sqlparser.String(stmt) + return sql, nil +} + // GetAction extracts the DDL action type from the online DDL statement func (onlineDDL *OnlineDDL) GetAction() (action sqlparser.DDLAction, err error) { if _, err := onlineDDL.GetRevertUUID(); err == nil { diff --git a/go/vt/schema/online_ddl_test.go b/go/vt/schema/online_ddl_test.go index b361ac1ae3a..fcf4634ac49 100644 --- a/go/vt/schema/online_ddl_test.go +++ b/go/vt/schema/online_ddl_test.go @@ -210,8 +210,8 @@ func TestNewOnlineDDL(t *testing.T) { NewDDLStrategySetting(DDLStrategyOnline, ""), NewDDLStrategySetting(DDLStrategyOnline, "-singleton"), } - require.False(t, strategies[0].IsSkipTopo()) - require.False(t, strategies[1].IsSkipTopo()) + require.True(t, strategies[0].IsSkipTopo()) + require.True(t, strategies[1].IsSkipTopo()) require.True(t, strategies[2].IsSkipTopo()) for _, ts := range tt { @@ -224,16 +224,11 @@ func TestNewOnlineDDL(t *testing.T) { return } assert.NoError(t, err) - if stgy.IsSkipTopo() { - // onlineDDL.SQL enriched with /*vt+ ... */ comment - assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(onlineDDL.UUID))) - assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(migrationContext))) - assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(string(stgy.Strategy)))) - } else { - assert.NotContains(t, onlineDDL.SQL, hex.EncodeToString([]byte(onlineDDL.UUID))) - assert.NotContains(t, onlineDDL.SQL, hex.EncodeToString([]byte(migrationContext))) - assert.NotContains(t, onlineDDL.SQL, hex.EncodeToString([]byte(string(stgy.Strategy)))) - } + require.True(t, stgy.IsSkipTopo(), "IsSkipTopo() should always be true") + // onlineDDL.SQL enriched with /*vt+ ... */ comment + assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(onlineDDL.UUID))) + assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(migrationContext))) + assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(string(stgy.Strategy)))) }) } }) @@ -297,7 +292,8 @@ func TestNewOnlineDDLs(t *testing.T) { sqls := []string{} for _, onlineDDL := range onlineDDLs { - sql := onlineDDL.SQL + sql, err := onlineDDL.sqlWithoutComments() + assert.NoError(t, err) sql = strings.ReplaceAll(sql, "\n", "") sql = strings.ReplaceAll(sql, "\t", "") sqls = append(sqls, sql) From c679d4012b2287746b9583ff0fe88c4d6b00431d Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 11 Jul 2021 15:56:54 +0300 Subject: [PATCH 3/3] no need to specify -skip-topo in test files Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go | 4 ++-- .../vrepl_stress/onlineddl_vrepl_mini_stress_test.go | 2 +- .../onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go b/go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go index 8efe3bbc708..17cbcc51f40 100644 --- a/go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go +++ b/go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go @@ -311,13 +311,13 @@ func TestSchemaChange(t *testing.T) { onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, true) }) t.Run("Online CREATE no PK table, vtgate", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, noPKCreateTableStatement, "gh-ost --skip-topo", "vtgate", "online_ddl_create_col") + uuid := testOnlineDDLStatement(t, noPKCreateTableStatement, "gh-ost", "vtgate", "online_ddl_create_col") onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false) onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, false) }) t.Run("Fail ALTER for no PK table, vtgate", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost --skip-topo", "vtgate", "") + uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost", "vtgate", "") onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed) rs := onlineddl.ReadMigrations(t, &vtParams, uuid) diff --git a/go/test/endtoend/onlineddl/vrepl_stress/onlineddl_vrepl_mini_stress_test.go b/go/test/endtoend/onlineddl/vrepl_stress/onlineddl_vrepl_mini_stress_test.go index 45e422ccab4..de4545dad0c 100644 --- a/go/test/endtoend/onlineddl/vrepl_stress/onlineddl_vrepl_mini_stress_test.go +++ b/go/test/endtoend/onlineddl/vrepl_stress/onlineddl_vrepl_mini_stress_test.go @@ -88,7 +88,7 @@ var ( opOrder int64 opOrderMutex sync.Mutex - onlineDDLStrategy = "online -vreplication-test-suite -skip-topo" + onlineDDLStrategy = "online -vreplication-test-suite" hostname = "localhost" keyspaceName = "ks" shards []cluster.Shard diff --git a/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go b/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go index 3af42dff93b..27357451e0d 100644 --- a/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go +++ b/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go @@ -46,7 +46,7 @@ var ( clusterInstance *cluster.LocalProcessCluster vtParams mysql.ConnParams evaluatedMysqlParams *mysql.ConnParams - ddlStrategy = "online -skip-topo -vreplication-test-suite" + ddlStrategy = "online -vreplication-test-suite" waitForMigrationTimeout = 20 * time.Second hostname = "localhost"