Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ApplySchema: deprecate '--skip_preflight' flag #10716

Merged
merged 9 commits into from
Feb 28, 2023
Merged

ApplySchema: deprecate '--skip_preflight' flag #10716

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c7857c1
ApplySchema: deprecate '-skip_preflight' flag
shlomi-noach Jul 17, 2022
4e7cdbf
Merge branch 'main' into apply-schema-deprecate-skip-preflight
shlomi-noach Jul 20, 2022
6c63668
Merge branch 'main' into apply-schema-deprecate-skip-preflight
shlomi-noach Jul 24, 2022
5c80d8d
warn if --skip_preflight is provided
shlomi-noach Jul 24, 2022
92b84bc
remove -skip_preflight from tests
shlomi-noach Jul 25, 2022
1e9d657
Merge branch 'main' into apply-schema-deprecate-skip-preflight
shlomi-noach Jul 27, 2022
aadaa8e
MarkDeprecated, fix usage
shlomi-noach Jul 27, 2022
5db8806
merge main, resolve conflict
shlomi-noach Feb 27, 2023
2b2cfd5
remove skip_preflight test
shlomi-noach Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions go/cmd/vtctldclient/command/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import (
var (
// ApplySchema makes an ApplySchema gRPC call to a vtctld.
ApplySchema = &cobra.Command{
Use: "ApplySchema [--allow-long-unavailability] [--ddl-strategy <strategy>] [--uuid <uuid> ...] [--migration-context <context>] [--wait-replicas-timeout <duration>] [--skip-preflight] [--caller-id <caller_id>] {--sql-file <file> | --sql <sql>} <keyspace>",
Use: "ApplySchema [--allow-long-unavailability] [--ddl-strategy <strategy>] [--uuid <uuid> ...] [--migration-context <context>] [--wait-replicas-timeout <duration>] [--caller-id <caller_id>] {--sql-file <file> | --sql <sql>} <keyspace>",
Short: "Applies the schema change to the specified keyspace on every primary, running in parallel on all shards. The changes are then propagated to replicas via replication.",
Long: `Applies the schema change to the specified keyspace on every primary, running in parallel on all shards. The changes are then propagated to replicas via replication.

Expand Down Expand Up @@ -141,7 +141,7 @@ func commandApplySchema(cmd *cobra.Command, args []string) error {
AllowLongUnavailability: applySchemaOptions.AllowLongUnavailability,
DdlStrategy: applySchemaOptions.DDLStrategy,
Sql: parts,
SkipPreflight: applySchemaOptions.SkipPreflight,
SkipPreflight: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments I made in #10717 (comment), but for this flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments addressed

UuidList: applySchemaOptions.UUIDList,
MigrationContext: applySchemaOptions.MigrationContext,
WaitReplicasTimeout: protoutil.DurationToProto(applySchemaOptions.WaitReplicasTimeout),
Expand Down Expand Up @@ -286,12 +286,12 @@ func commandReloadSchemaShard(cmd *cobra.Command, args []string) error {
}

func init() {
ApplySchema.Flags().MarkDeprecated("--skip-preflight", "Deprecated. Assumed to be always 'true'")
ApplySchema.Flags().BoolVar(&applySchemaOptions.AllowLongUnavailability, "allow-long-unavailability", false, "Allow large schema changes which incur a longer unavailability of the database.")
ApplySchema.Flags().StringVar(&applySchemaOptions.DDLStrategy, "ddl-strategy", string(schema.DDLStrategyDirect), "Online DDL strategy, compatible with @@ddl_strategy session variable (examples: 'gh-ost', 'pt-osc', 'gh-ost --max-load=Threads_running=100'.")
ApplySchema.Flags().StringSliceVar(&applySchemaOptions.UUIDList, "uuid", nil, "Optional, comma-delimited, repeatable, explicit UUIDs for migration. If given, must match number of DDL changes.")
ApplySchema.Flags().StringVar(&applySchemaOptions.MigrationContext, "migration-context", "", "For Online DDL, optionally supply a custom unique string used as context for the migration(s) in this command. By default a unique context is auto-generated by Vitess.")
ApplySchema.Flags().DurationVar(&applySchemaOptions.WaitReplicasTimeout, "wait-replicas-timeout", wrangler.DefaultWaitReplicasTimeout, "Amount of time to wait for replicas to receive the schema change via replication.")
ApplySchema.Flags().BoolVar(&applySchemaOptions.SkipPreflight, "skip-preflight", false, "Skip pre-apply schema checks, and directly forward schema change query to shards.")
ApplySchema.Flags().StringVar(&applySchemaOptions.CallerID, "caller-id", "", "Effective caller ID used for the operation and should map to an ACL name which grants this identity the necessary permissions to perform the operation (this is only necessary when strict table ACLs are used).")
ApplySchema.Flags().StringSliceVar(&applySchemaOptions.SQL, "sql", nil, "Semicolon-delimited, repeatable SQL commands to apply. Exactly one of --sql|--sql-file is required.")
ApplySchema.Flags().StringVar(&applySchemaOptions.SQLFile, "sql-file", "", "Path to a file containing semicolon-delimited SQL commands to apply. Exactly one of --sql|--sql-file is required.")
Expand Down
6 changes: 1 addition & 5 deletions go/test/endtoend/cluster/vtctlclient_process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ type VtctlClientProcess struct {
type VtctlClientParams struct {
DDLStrategy string
MigrationContext string
SkipPreflight bool
UUIDList string
CallerID string
}
Expand Down Expand Up @@ -88,9 +87,6 @@ func (vtctlclient *VtctlClientProcess) ApplySchemaWithOutput(Keyspace string, SQ
if params.UUIDList != "" {
args = append(args, "--uuid_list", params.UUIDList)
}
if params.SkipPreflight {
args = append(args, "--skip_preflight")
}

if params.CallerID != "" {
args = append(args, "--caller_id", params.CallerID)
Expand All @@ -101,7 +97,7 @@ func (vtctlclient *VtctlClientProcess) ApplySchemaWithOutput(Keyspace string, SQ

// ApplySchema applies SQL schema to the keyspace
func (vtctlclient *VtctlClientProcess) ApplySchema(Keyspace string, SQL string) error {
message, err := vtctlclient.ApplySchemaWithOutput(Keyspace, SQL, VtctlClientParams{DDLStrategy: "direct -allow-zero-in-date", SkipPreflight: true})
message, err := vtctlclient.ApplySchemaWithOutput(Keyspace, SQL, VtctlClientParams{DDLStrategy: "direct -allow-zero-in-date"})

return vterrors.Wrap(err, message)
}
Expand Down
2 changes: 1 addition & 1 deletion go/test/endtoend/onlineddl/declarative/onlineddl_declarative_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str
uuid = row.AsString("uuid", "")
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, MigrationContext: migrationContext, SkipPreflight: true})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, MigrationContext: migrationContext})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand Down
2 changes: 1 addition & 1 deletion go/test/endtoend/onlineddl/revertible/onlineddl_revertible_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str
}
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand Down
6 changes: 3 additions & 3 deletions go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ func TestSchemaChange(t *testing.T) {

t.Run("Idempotent submission, retry failed migration", func(t *testing.T) {
uuid := "00000000_1111_2222_3333_444444444444"
overrideVtctlParams = &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, UUIDList: uuid, MigrationContext: "idempotent:1111-2222-3333"}
overrideVtctlParams = &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, UUIDList: uuid, MigrationContext: "idempotent:1111-2222-3333"}
defer func() { overrideVtctlParams = nil }()
// create a migration and cancel it. We don't let it complete. We want it in "failed" state
t.Run("start and fail migration", func(t *testing.T) {
Expand Down Expand Up @@ -580,7 +580,7 @@ func testOnlineDDLStatement(t *testing.T, ddlStatement string, ddlStrategy strin
}
}
} else {
params := &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true}
params := &cluster.VtctlClientParams{DDLStrategy: ddlStrategy}
if overrideVtctlParams != nil {
params = overrideVtctlParams
}
Expand Down Expand Up @@ -620,7 +620,7 @@ func testRevertMigration(t *testing.T, revertUUID string, ddlStrategy, executeSt
}
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: ddlStrategy})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand Down
4 changes: 2 additions & 2 deletions go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str
}
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, MigrationContext: migrationContext})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, MigrationContext: migrationContext})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand Down Expand Up @@ -364,7 +364,7 @@ func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string
}
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, MigrationContext: migrationContext})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, MigrationContext: migrationContext})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand Down
2 changes: 1 addition & 1 deletion go/test/endtoend/onlineddl/vtctlutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ import (
func CheckCancelAllMigrationsViaVtctl(t *testing.T, vtctlclient *cluster.VtctlClientProcess, keyspace string) {
cancelQuery := "alter vitess_migration cancel all"

_, err := vtctlclient.ApplySchemaWithOutput(keyspace, cancelQuery, cluster.VtctlClientParams{SkipPreflight: true})
_, err := vtctlclient.ApplySchemaWithOutput(keyspace, cancelQuery, cluster.VtctlClientParams{})
assert.NoError(t, err)
}
2 changes: 1 addition & 1 deletion go/test/endtoend/vreplication/resharding_workflows_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ func createAdditionalCustomerShards(t *testing.T, shards string) {
}

func tstApplySchemaOnlineDDL(t *testing.T, sql string, keyspace string) {
err := vc.VtctlClient.ExecuteCommand("ApplySchema", "--", "--skip_preflight", "--ddl_strategy=online",
err := vc.VtctlClient.ExecuteCommand("ApplySchema", "--", "--ddl_strategy=online",
"--sql", sql, keyspace)
require.NoError(t, err, fmt.Sprintf("ApplySchema Error: %s", err))
}
4 changes: 2 additions & 2 deletions go/test/endtoend/vtgate/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ func testDropNonExistentTables(t *testing.T) {
// we test with different 'direct' strategy options
func testCreateInvalidView(t *testing.T) {
for _, ddlStrategy := range []string{"direct", "direct -allow-zero-in-date"} {
createInvalidView := "CREATE OR REPLACE VIEW invalid_view AS SELECT * FROM nonexistent_table;"
output, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ApplySchema", "--", "-skip_preflight", "-ddl_strategy", ddlStrategy, "--sql", createInvalidView, keyspaceName)
createInvalidView := "CREATE OR REPLACE VIEW invalid_view AS SELECT * FROM nonexistent_table"
output, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ApplySchema", "--", "-ddl_strategy", ddlStrategy, "--sql", createInvalidView, keyspaceName)
require.Error(t, err)
assert.Contains(t, output, "doesn't exist (errno 1146)")
}
Expand Down
20 changes: 0 additions & 20 deletions go/vt/schemamanager/tablet_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ type TabletExecutor struct {
waitReplicasTimeout time.Duration
ddlStrategySetting *schema.DDLStrategySetting
uuids []string
skipPreflight bool
}

// NewTabletExecutor creates a new TabletExecutor instance
Expand Down Expand Up @@ -108,11 +107,6 @@ func (exec *TabletExecutor) hasProvidedUUIDs() bool {
return len(exec.uuids) != 0
}

// SkipPreflight disables preflight checks
func (exec *TabletExecutor) SkipPreflight() {
exec.skipPreflight = true
}

// Open opens a connection to the primary for every shard.
func (exec *TabletExecutor) Open(ctx context.Context, keyspace string) error {
if !exec.isClosed {
Expand Down Expand Up @@ -268,14 +262,6 @@ func (exec *TabletExecutor) detectBigSchemaChanges(ctx context.Context, parsedDD
return false, nil
}

func (exec *TabletExecutor) preflightSchemaChanges(ctx context.Context, sqls []string) error {
if exec.skipPreflight {
return nil
}
_, err := exec.tmc.PreflightSchema(ctx, exec.tablets[0], sqls)
return err
}

// executeSQL executes a single SQL statement either as online DDL or synchronously on all tablets.
// In online DDL case, the query may be exploded into multiple queries during
func (exec *TabletExecutor) executeSQL(ctx context.Context, sql string, providedUUID string, execResult *ExecuteResult) (executedAsynchronously bool, err error) {
Expand Down Expand Up @@ -347,12 +333,6 @@ func (exec *TabletExecutor) Execute(ctx context.Context, sqls []string) *Execute
}
}()

// Make sure the schema changes introduce a table definition change.
if err := exec.preflightSchemaChanges(ctx, sqls); err != nil {
execResult.ExecutorErr = err.Error()
return &execResult
}

if exec.hasProvidedUUIDs() && len(exec.uuids) != len(sqls) {
execResult.ExecutorErr = fmt.Sprintf("provided %v UUIDs do not match number of DDLs %v", len(exec.uuids), len(sqls))
return &execResult
Expand Down
5 changes: 0 additions & 5 deletions go/vt/vtctl/grpcvtctldserver/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ func (s *VtctldServer) ApplySchema(ctx context.Context, req *vtctldatapb.ApplySc
defer span.Finish()

span.Annotate("keyspace", req.Keyspace)
span.Annotate("skip_preflight", req.SkipPreflight)
span.Annotate("ddl_strategy", req.DdlStrategy)

if len(req.Sql) == 0 {
Expand Down Expand Up @@ -215,10 +214,6 @@ func (s *VtctldServer) ApplySchema(ctx context.Context, req *vtctldatapb.ApplySc
if req.AllowLongUnavailability {
executor.AllowBigSchemaChange()
}
if req.SkipPreflight {
executor.SkipPreflight()
}

if err := executor.SetDDLStrategy(req.DdlStrategy); err != nil {
return resp, vterrors.Wrapf(err, "invalid DdlStrategy: %s", req.DdlStrategy)
}
Expand Down
12 changes: 7 additions & 5 deletions go/vt/vtctl/vtctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,8 +611,8 @@ var commands = []commandGroup{
{
name: "ApplySchema",
method: commandApplySchema,
params: "[--allow_long_unavailability] [--wait_replicas_timeout=10s] [--ddl_strategy=<ddl_strategy>] [--uuid_list=<comma_separated_uuids>] [--migration_context=<unique-request-context>] [--skip_preflight] {--sql=<sql> || --sql-file=<filename>} <keyspace>",
help: "Applies the schema change to the specified keyspace on every primary, running in parallel on all shards. The changes are then propagated to replicas via replication. If --allow_long_unavailability is set, schema changes affecting a large number of rows (and possibly incurring a longer period of unavailability) will not be rejected. -ddl_strategy is used to instruct migrations via vreplication, gh-ost or pt-osc with optional parameters. -migration_context allows the user to specify a custom request context for online DDL migrations. If -skip_preflight, SQL goes directly to shards without going through sanity checks.",
params: "[--allow_long_unavailability] [--wait_replicas_timeout=10s] [--ddl_strategy=<ddl_strategy>] [--uuid_list=<comma_separated_uuids>] [--migration_context=<unique-request-context>] {--sql=<sql> || --sql-file=<filename>} <keyspace>",
help: "Applies the schema change to the specified keyspace on every primary, running in parallel on all shards. The changes are then propagated to replicas via replication. If --allow_long_unavailability is set, schema changes affecting a large number of rows (and possibly incurring a longer period of unavailability) will not be rejected. -ddl_strategy is used to instruct migrations via vreplication, gh-ost or pt-osc with optional parameters. -migration_context allows the user to specify a custom request context for online DDL migrations.",
},
{
name: "CopySchemaShard",
Expand Down Expand Up @@ -3064,7 +3064,7 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *fl
migrationContext := subFlags.String("migration_context", "", "For Online DDL, optionally supply a custom unique string used as context for the migration(s) in this command. By default a unique context is auto-generated by Vitess")
requestContext := subFlags.String("request_context", "", "synonym for --migration_context")
waitReplicasTimeout := subFlags.Duration("wait_replicas_timeout", wrangler.DefaultWaitReplicasTimeout, "The amount of time to wait for replicas to receive the schema change via replication.")
skipPreflight := subFlags.Bool("skip_preflight", false, "Skip pre-apply schema checks, and directly forward schema change query to shards")
skipPreflight := subFlags.Bool("skip_preflight", false, "Deprecated. Always assumed to be 'true'")

callerID := subFlags.String("caller_id", "", "This is the effective caller ID used for the operation and should map to an ACL name which grants this identity the necessary permissions to perform the operation (this is only necessary when strict table ACLs are used)")
if err := subFlags.Parse(args); err != nil {
Expand All @@ -3073,7 +3073,9 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *fl
if subFlags.NArg() != 1 {
return fmt.Errorf("the <keyspace> argument is required for the commandApplySchema command")
}

if *skipPreflight {
log.Warningf("--skip_preflight flag is deprecated. Always assumed to be 'true'")
}
// v14 deprecates `--skip-topo` flag. This check will be removed in v15
if settings, _ := schema.ParseDDLStrategy(*ddlStrategy); settings != nil && settings.IsSkipTopoFlag() {
deprecationMessage := `--skip-topo flag is deprecated and will be removed in v15`
Expand Down Expand Up @@ -3110,7 +3112,7 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *fl
AllowLongUnavailability: *allowLongUnavailability,
DdlStrategy: *ddlStrategy,
Sql: parts,
SkipPreflight: *skipPreflight,
SkipPreflight: true,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should be deprecating the protobuf field for this as well? I'm not certain about that though - whether we do this now and deprecate the protobuf field in the next release.
@ajm188 what do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

i know i just said "yes" on the other PR, but i think we need to wait one more cycle, since old tablets will be consuming this field, which will take the zero value (false) instead of what we unconditionally pass here (true).

so deprecate flag + always pass true (v15) => remove flag + deprecate protobuf field + tablets ignore value and always skip-preflight (v16)

UuidList: textutil.SplitDelimitedList(*uuidList),
MigrationContext: *migrationContext,
WaitReplicasTimeout: protoutil.DurationToProto(*waitReplicasTimeout),
Expand Down