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

LookupVindex: Implement internalize command for lookup vindexes #17429

Merged
merged 16 commits into from
Jan 22, 2025
Merged

LookupVindex: Implement internalize command for lookup vindexes #17429

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3ab44c3
LookupVindex: Implement internalize command for lookup vindexes
beingnoble03 Dec 24, 2024
944c39c
LookupVindex: Implement LookupVindexComplete
beingnoble03 Dec 26, 2024
7038c78
Merge branch 'main' into lookup-vindex-internalize
beingnoble03 Dec 29, 2024
128198d
test(LookupVindex): Add tests for internalize and complete
beingnoble03 Dec 29, 2024
983a415
Add more e2e tests and helper func to get vindex
beingnoble03 Jan 5, 2025
ee296d2
Add delete flag for externalize command to continue old behaviour
beingnoble03 Jan 8, 2025
f11a0a7
test: Fix failing TestInternalizeLookupVindex sub-tests
beingnoble03 Jan 8, 2025
f6f42ba
Extend support for stream message in UpdateVReplicationWorkflow
beingnoble03 Jan 9, 2025
f63d9f3
test: Add/Fix unit tests for updating message with UpdateVReplication…
beingnoble03 Jan 9, 2025
18b7de1
test: Add unit test when we want workflow deleted after externalizing
beingnoble03 Jan 9, 2025
e866118
Check if the vindex has an owner while validating externalized vindex
beingnoble03 Jan 14, 2025
1631cd2
test: Fix unit tests for LookupVindexComplete
beingnoble03 Jan 14, 2025
2da390d
fix: Fix user facing text
beingnoble03 Jan 19, 2025
db3c825
Merge branch 'main' into lookup-vindex-internalize
beingnoble03 Jan 19, 2025
dca9d30
Merge main into lookup-vindex-internalize
beingnoble03 Jan 19, 2025
5c0cda5
Merge main into lookup-vindex-internalize
beingnoble03 Jan 22, 2025
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
108 changes: 104 additions & 4 deletions go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@ var (

externalizeOptions = struct {
Keyspace string
Delete bool
}{}

internalizeOptions = struct {
Keyspace string
}{}

completeOptions = struct {
Keyspace string
}{}

parseAndValidateCreate = func(cmd *cobra.Command, args []string) error {
Expand Down Expand Up @@ -142,6 +151,18 @@ var (
RunE: commandCancel,
}

// complete makes a LookupVindexComplete call to a vtctld.
complete = &cobra.Command{
mattlord marked this conversation as resolved.
Show resolved Hide resolved
Use: "complete",
Short: "Complete the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be deleted.",
mattlord marked this conversation as resolved.
Show resolved Hide resolved
Example: `vtctldclient --server localhost:15999 LookupVindex --name corder_lookup_vdx --table-keyspace customer complete`,
SilenceUsage: true,
DisableFlagsInUseLine: true,
Aliases: []string{"Complete"},
Args: cobra.NoArgs,
RunE: commandComplete,
}

// create makes a LookupVindexCreate call to a vtctld.
create = &cobra.Command{
Use: "create",
Expand All @@ -158,7 +179,7 @@ var (
// externalize makes a LookupVindexExternalize call to a vtctld.
externalize = &cobra.Command{
Use: "externalize",
Short: "Externalize the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be deleted.",
Short: "Externalize the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be stopped/deleted.",
Copy link
Contributor

@mattlord mattlord Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This text is a bit off IMO. If we're not able to set the owner (the backing table) as part of the externalize then it's an error. No? Unless I'm missing something, then I think this is better:

"Externalize the Vindex so that it can be used for queries and stop (or optionally delete) the workflow."

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we don't return an error if the vindex is unowned while externalizing, instead we just remove the write_only param from the vindex. This is the old behaviour that we follow in LookupVindexExternalize. Do you think we should change this behaviour (we can return error for unowned vindexes)?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fine to leave it as is then. Been a while since I was in this code so had forgotten. Thanks!

Example: `vtctldclient --server localhost:15999 LookupVindex --name corder_lookup_vdx --table-keyspace customer externalize`,
SilenceUsage: true,
DisableFlagsInUseLine: true,
Expand All @@ -167,6 +188,18 @@ var (
RunE: commandExternalize,
}

// internalize makes a LookupVindexInternalize call to a vtctld.
internalize = &cobra.Command{
Use: "internalize",
Short: "Internalize the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be started.",
mattlord marked this conversation as resolved.
Show resolved Hide resolved
Example: `vtctldclient --server localhost:15999 LookupVindex --name corder_lookup_vdx --table-keyspace customer internalize`,
SilenceUsage: true,
DisableFlagsInUseLine: true,
Aliases: []string{"Internalize"},
Args: cobra.NoArgs,
RunE: commandInternalize,
}

// show makes a GetWorkflows call to a vtctld.
show = &cobra.Command{
Use: "show",
Expand Down Expand Up @@ -199,6 +232,35 @@ func commandCancel(cmd *cobra.Command, args []string) error {
return nil
}

func commandComplete(cmd *cobra.Command, args []string) error {
if completeOptions.Keyspace == "" {
completeOptions.Keyspace = baseOptions.TableKeyspace
}
cli.FinishedParsing(cmd)

resp, err := common.GetClient().LookupVindexComplete(common.GetCommandCtx(), &vtctldatapb.LookupVindexCompleteRequest{
Keyspace: completeOptions.Keyspace,
// The name of the workflow and lookup vindex.
Name: baseOptions.Name,
// Where the lookup table and VReplication workflow were created.
TableKeyspace: baseOptions.TableKeyspace,
})

if err != nil {
return err
}

output := fmt.Sprintf("LookupVindex %s has been completed", baseOptions.Name)
if resp.WorkflowDeleted {
output = output + " and the VReplication workflow has been deleted."
} else {
output = output + ". The VReplication workflow hasn't been stopped as the vindex is unowned."
}
mattlord marked this conversation as resolved.
Show resolved Hide resolved
fmt.Println(output)

return nil
}

func commandCreate(cmd *cobra.Command, args []string) error {
tsp := common.GetTabletSelectionPreference(cmd)
cli.FinishedParsing(cmd)
Expand Down Expand Up @@ -236,15 +298,46 @@ func commandExternalize(cmd *cobra.Command, args []string) error {
Name: baseOptions.Name,
// Where the lookup table and VReplication workflow were created.
TableKeyspace: baseOptions.TableKeyspace,
// Delete the workflow after externalizing, instead of stopping.
DeleteWorkflow: externalizeOptions.Delete,
})

if err != nil {
return err
}

output := fmt.Sprintf("LookupVindex %s has been externalized", baseOptions.Name)
if resp.WorkflowDeleted {
output = output + fmt.Sprintf(" and the %s VReplication workflow has been deleted", baseOptions.Name)
if resp.WorkflowStopped {
output = output + " and the VReplication workflow has been stopped."
} else if resp.WorkflowDeleted {
output = output + " and the VReplication workflow has been deleted."
}
fmt.Println(output)

return nil
}

func commandInternalize(cmd *cobra.Command, args []string) error {
if internalizeOptions.Keyspace == "" {
internalizeOptions.Keyspace = baseOptions.TableKeyspace
}
cli.FinishedParsing(cmd)

resp, err := common.GetClient().LookupVindexInternalize(common.GetCommandCtx(), &vtctldatapb.LookupVindexInternalizeRequest{
Keyspace: internalizeOptions.Keyspace,
// The name of the workflow and lookup vindex.
Name: baseOptions.Name,
// Where the lookup table and VReplication workflow were created.
TableKeyspace: baseOptions.TableKeyspace,
})

if err != nil {
return err
}

output := fmt.Sprintf("LookupVindex %s has been internalized", baseOptions.Name)
if resp.WorkflowStarted {
output = output + " and the VReplication workflow has been started."
mattlord marked this conversation as resolved.
Show resolved Hide resolved
}
fmt.Println(output)

Expand Down Expand Up @@ -304,12 +397,19 @@ func registerCommands(root *cobra.Command) {
// for the VReplication workflow used.
base.AddCommand(show)

// This will also delete the VReplication workflow if the
// This will also stop the VReplication workflow if the
// vindex has an owner as the lookup vindex will then be
// managed by VTGate.
externalize.Flags().StringVar(&externalizeOptions.Keyspace, "keyspace", "", "The keyspace containing the Lookup Vindex. If no value is specified then the table-keyspace will be used.")
externalize.Flags().BoolVar(&externalizeOptions.Delete, "delete", false, "Delete the VReplication workflow after externalizing LookupVindex, instead of stopping (default false).")
mattlord marked this conversation as resolved.
Show resolved Hide resolved
base.AddCommand(externalize)

internalize.Flags().StringVar(&internalizeOptions.Keyspace, "keyspace", "", "The keyspace containing the Lookup Vindex. If no value is specified then the table-keyspace will be used.")
base.AddCommand(internalize)

complete.Flags().StringVar(&completeOptions.Keyspace, "keyspace", "", "The keyspace containing the Lookup Vindex. If no value is specified then the table-keyspace will be used.")
base.AddCommand(complete)

// The cancel command deletes the VReplication workflow used
// to backfill the lookup vindex. It ends up making a
// WorkflowDelete VtctldServer call.
Expand Down
49 changes: 44 additions & 5 deletions go/test/endtoend/vreplication/lookup_vindex_helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,18 @@ func (lv *lookupVindex) create() {
err := vc.VtctldClient.ExecuteCommand(args...)
require.NoError(lv.t, err, "error executing LookupVindex create: %v", err)
waitForWorkflowState(lv.t, vc, fmt.Sprintf("%s.%s", lv.ownerTableKeyspace, lv.name), binlogdatapb.VReplicationWorkflowState_Running.String())
lv.expectWriteOnly(true)
lv.expectParamsAndOwner(true)
}

func (lv *lookupVindex) cancel() {
panic("not implemented")
args := []string{
"LookupVindex",
"--name", lv.name,
"--table-keyspace=" + lv.ownerTableKeyspace,
"cancel",
}
err := vc.VtctldClient.ExecuteCommand(args...)
require.NoError(lv.t, err, "error executing LookupVindex complete: %v", err)
}

func (lv *lookupVindex) externalize() {
Expand All @@ -83,20 +90,52 @@ func (lv *lookupVindex) externalize() {
}
err := vc.VtctldClient.ExecuteCommand(args...)
require.NoError(lv.t, err, "error executing LookupVindex externalize: %v", err)
lv.expectWriteOnly(false)
lv.expectParamsAndOwner(false)
}

func (lv *lookupVindex) internalize() {
args := []string{
"LookupVindex",
"--name", lv.name,
"--table-keyspace=" + lv.ownerTableKeyspace,
"internalize",
"--keyspace=" + lv.tableKeyspace,
}
err := vc.VtctldClient.ExecuteCommand(args...)
require.NoError(lv.t, err, "error executing LookupVindex internalize: %v", err)
lv.expectParamsAndOwner(true)
}

func (lv *lookupVindex) complete() {
args := []string{
"LookupVindex",
"--name", lv.name,
"--table-keyspace=" + lv.ownerTableKeyspace,
"complete",
"--keyspace=" + lv.tableKeyspace,
}
err := vc.VtctldClient.ExecuteCommand(args...)
require.NoError(lv.t, err, "error executing LookupVindex complete: %v", err)
lv.expectParamsAndOwner(false)
}

func (lv *lookupVindex) show() error {
return nil
}

func (lv *lookupVindex) expectWriteOnly(expected bool) {
func (lv *lookupVindex) expectParamsAndOwner(expectedWriteOnlyParam bool) {
vschema, err := vc.VtctldClient.ExecuteCommandWithOutput("GetVSchema", lv.ownerTableKeyspace)
require.NoError(lv.t, err, "error executing GetVSchema: %v", err)
vdx := gjson.Get(vschema, fmt.Sprintf("vindexes.%s", lv.name))
require.NotNil(lv.t, vdx, "lookup vindex %s not found", lv.name)

expectedOwner, expectedFrom, expectedTo := lv.ownerTable, strings.Join(lv.columns, ","), "keyspace_id"
require.Equal(lv.t, expectedOwner, vdx.Get("owner").String(), "expected 'owner' parameter to be %s", expectedOwner)
require.Equal(lv.t, expectedFrom, vdx.Get("params.from").String(), "expected 'from' parameter to be %s", expectedFrom)
require.Equal(lv.t, expectedTo, vdx.Get("params.to").String(), "expected 'to' parameter to be %s", expectedTo)

want := ""
if expected {
if expectedWriteOnlyParam {
want = "true"
}
require.Equal(lv.t, want, vdx.Get("params.write_only").String(), "expected write_only parameter to be %s", want)
Expand Down
Loading
Loading