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

vtctl OnlineDDL: complete command set #12963

Merged
merged 5 commits into from
Jul 13, 2023
Merged

vtctl OnlineDDL: complete command set #12963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
039acd8
vtctl OnlineDDL: complete commands
shlomi-noach Apr 25, 2023
e970022
cleanup
shlomi-noach Apr 25, 2023
b0bb74f
refactor, normalize all commands in a single parsing function
shlomi-noach Jul 12, 2023
1357cc0
Merge branch 'main' into vtctl-onlineddl-throttle
shlomi-noach Jul 12, 2023
27d4aae
better error message
shlomi-noach Jul 12, 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
91 changes: 64 additions & 27 deletions go/vt/vtctl/vtctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,8 +608,19 @@ var commands = []commandGroup{
" \nvtctl OnlineDDL test_keyspace show running" +
" \nvtctl OnlineDDL test_keyspace show complete" +
" \nvtctl OnlineDDL test_keyspace show failed" +
" \nvtctl OnlineDDL test_keyspace cleanup 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace retry 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace cancel 82fa54ac_e83e_11ea_96b7_f875a4d24e90",
" \nvtctl OnlineDDL test_keyspace cancel 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace cancel-all" +
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO cancel all is more intuitive, but it's a matter of taste and don't feel strongly about it. The benefit is that it's a single command/signature with the second argument being UUID/all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cancel all (and likewise complete all, throttle all, etc.) are now supported. Refactored the code a bit to generalize the parsing of the command.

" \nvtctl OnlineDDL test_keyspace launch 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace launch-all" +
" \nvtctl OnlineDDL test_keyspace complete 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace complete-all" +
" \nvtctl OnlineDDL test_keyspace throttle 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace throttle-all" +
" \nvtctl OnlineDDL test_keyspace unthrottle 82fa54ac_e83e_11ea_96b7_f875a4d24e90" +
" \nvtctl OnlineDDL test_keyspace unthrottle-all" +
"",
},
{
name: "ValidateVersionShard",
Expand Down Expand Up @@ -2921,6 +2932,34 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *pf
return nil
}

func generateOnlineDDLQuery(command string, arg string, allSupported bool) (string, error) {
// Accept inputs like so:
// "launch", "all"
// "launch", <uuid>
// "launch-all", <empty>
if tokens := strings.Split(command, "-"); len(tokens) == 2 && tokens[1] == "all" {
// command is e.g. "launch-all"
if arg != "" {
return "", fmt.Errorf("UUID not allowed in '%s' command", command)
}
// transform "launch-all" into "launch", "all"
command = tokens[0]
arg = "all"
}
switch arg {
case "":
return "", fmt.Errorf("UUID|all required")
case "all":
if !allSupported {
return "", fmt.Errorf("'all' not supported for '%s' command", command)
}
return fmt.Sprintf(`alter vitess_migration %s all`, command), nil
default:
query := `alter vitess_migration %a ` + command
return sqlparser.ParseAndBind(query, sqltypes.StringBindVariable(arg))
}
}

func commandOnlineDDL(ctx context.Context, wr *wrangler.Wrangler, subFlags *pflag.FlagSet, args []string) error {
json := subFlags.Bool("json", false, "Output JSON instead of human-readable table")
orderBy := subFlags.String("order", "ascending", "Sort the results by `id` property of the Schema migration (default is ascending. Allowed values are `ascending` or `descending`.")
Expand All @@ -2944,7 +2983,7 @@ func commandOnlineDDL(ctx context.Context, wr *wrangler.Wrangler, subFlags *pfla

applySchemaQuery := ""
executeFetchQuery := ""
var bindErr error
var err error
switch command {
case "show":
condition := ""
Expand All @@ -2960,12 +2999,12 @@ func commandOnlineDDL(ctx context.Context, wr *wrangler.Wrangler, subFlags *pfla
string(schema.OnlineDDLStatusRunning),
string(schema.OnlineDDLStatusComplete),
string(schema.OnlineDDLStatusFailed):
condition, bindErr = sqlparser.ParseAndBind("migration_status=%a", sqltypes.StringBindVariable(arg))
condition, err = sqlparser.ParseAndBind("migration_status=%a", sqltypes.StringBindVariable(arg))
default:
if schema.IsOnlineDDLUUID(arg) {
condition, bindErr = sqlparser.ParseAndBind("migration_uuid=%a", sqltypes.StringBindVariable(arg))
condition, err = sqlparser.ParseAndBind("migration_uuid=%a", sqltypes.StringBindVariable(arg))
} else {
condition, bindErr = sqlparser.ParseAndBind("migration_context=%a", sqltypes.StringBindVariable(arg))
condition, err = sqlparser.ParseAndBind("migration_context=%a", sqltypes.StringBindVariable(arg))
}
}
order := " order by `id` "
Expand All @@ -2984,31 +3023,29 @@ func commandOnlineDDL(ctx context.Context, wr *wrangler.Wrangler, subFlags *pfla
executeFetchQuery = fmt.Sprintf(`select
*
from _vt.schema_migrations where %s %s %s`, condition, order, skipLimit)
case "retry":
if arg == "" {
return fmt.Errorf("UUID required")
}
applySchemaQuery, bindErr = sqlparser.ParseAndBind(`alter vitess_migration %a retry`, sqltypes.StringBindVariable(arg))
case "complete":
if arg == "" {
return fmt.Errorf("UUID required")
}
applySchemaQuery, bindErr = sqlparser.ParseAndBind(`alter vitess_migration %a complete`, sqltypes.StringBindVariable(arg))
case "cancel":
if arg == "" {
return fmt.Errorf("UUID required")
}
applySchemaQuery, bindErr = sqlparser.ParseAndBind(`alter vitess_migration %a cancel`, sqltypes.StringBindVariable(arg))
case "cancel-all":
if arg != "" {
return fmt.Errorf("UUID not allowed in %s", command)
}
applySchemaQuery = `alter vitess_migration cancel all`
case
"retry",
"cleanup":
// Do not support 'ALL' argument
applySchemaQuery, err = generateOnlineDDLQuery(command, arg, false)
case
"launch",
"launch-all",
"complete",
"complete-all",
"cancel",
"cancel-all",
"throttle",
"throttle-all",
"unthrottle",
"unthrottle-all":
// Support 'ALL' argument
applySchemaQuery, err = generateOnlineDDLQuery(command, arg, true)
default:
return fmt.Errorf("Unknown OnlineDDL command: %s", command)
}
if bindErr != nil {
return fmt.Errorf("Error generating OnlineDDL query: %+v", bindErr)
if err != nil {
return fmt.Errorf("Error generating OnlineDDL query: %+v", err)
}

if applySchemaQuery != "" {
Expand Down
107 changes: 107 additions & 0 deletions go/vt/vtctl/vtctl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

"github.com/spf13/pflag"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/sqltypes"
Expand Down Expand Up @@ -360,3 +361,109 @@ func TestMoveTables(t *testing.T) {
})
}
}

func TestGenerateOnlineDDLQuery(t *testing.T) {
tcases := []struct {
cmd string
arg string
allSupported bool
expectError bool
expectQuery string
}{
{
"launch",
"all",
true,
false,
"alter vitess_migration launch all",
},
{
"launch-all",
"",
true,
false,
"alter vitess_migration launch all",
},
{
"launch",
"718169cc_1fea_11ee_82b1_0a43f95f28a3",
true,
false,
"alter vitess_migration '718169cc_1fea_11ee_82b1_0a43f95f28a3' launch",
},
{
"cancel",
"718169cc_1fea_11ee_82b1_0a43f95f28a3",
true,
false,
"alter vitess_migration '718169cc_1fea_11ee_82b1_0a43f95f28a3' cancel",
},
{
"unthrottle",
"718169cc_1fea_11ee_82b1_0a43f95f28a3",
true,
false,
"alter vitess_migration '718169cc_1fea_11ee_82b1_0a43f95f28a3' unthrottle",
},
{
"unthrottle",
"",
true,
true,
"",
},
{
"unthrottle-all",
"all",
true,
true,
"",
},
{
"unthrottle-all",
"718169cc_1fea_11ee_82b1_0a43f95f28a3",
true,
true,
"",
},
{
"retry",
"718169cc_1fea_11ee_82b1_0a43f95f28a3",
false,
false,
"alter vitess_migration '718169cc_1fea_11ee_82b1_0a43f95f28a3' retry",
},
{
"retry-all",
"718169cc_1fea_11ee_82b1_0a43f95f28a3",
false,
true,
"",
},
{
"retry-all",
"",
false,
true,
"",
},
{
"retry",
"all",
false,
true,
"",
},
}
for _, tcase := range tcases {
t.Run(fmt.Sprintf("%s %s", tcase.cmd, tcase.arg), func(t *testing.T) {
query, err := generateOnlineDDLQuery(tcase.cmd, tcase.arg, tcase.allSupported)
if tcase.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.Equal(t, tcase.expectQuery, query)
}
})
}
}
7 changes: 0 additions & 7 deletions go/vt/vttablet/onlineddl/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,13 +568,6 @@ const (
sqlFindProcess = "SELECT id, Info as info FROM information_schema.processlist WHERE id=%a AND Info LIKE %a"
)

const (
retryMigrationHint = "retry"
cancelMigrationHint = "cancel"
cancelAllMigrationHint = "cancel-all"
completeMigrationHint = "complete"
)

var (
sqlCreateOnlineDDLUser = []string{
`CREATE USER IF NOT EXISTS %s IDENTIFIED BY '%s'`,
Expand Down