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.

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

VReplication: Support automatically replacing auto_inc cols with sequences #16860

Merged
merged 26 commits into from
Oct 4, 2024
Merged
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e6487dd
Support automatically replacing auto_increment columns with sequences
mattlord Sep 27, 2024
33b4cc9
Various improvements
mattlord Sep 29, 2024
e56dd5b
Improve errors
mattlord Sep 29, 2024
1cb3de5
Add to flag handling test
mattlord Sep 29, 2024
f5eb4e2
Add unit test
mattlord Sep 29, 2024
b110120
Support creating missing sequence backing tables during init
mattlord Sep 30, 2024
6fc7fdc
Changes from self review
mattlord Sep 30, 2024
e89bbf7
Add unit test for auto table creation
mattlord Sep 30, 2024
2b28b2b
Minor error improvements
mattlord Sep 30, 2024
659929e
Add new test comment
mattlord Sep 30, 2024
cf99d54
Make auto generated sequence table name a const
mattlord Sep 30, 2024
292a422
Add comment for const
mattlord Sep 30, 2024
8d5087a
Address review comments
mattlord Oct 1, 2024
4cbfb20
Merge remote-tracking branch 'origin/main' into vrepl_replace_auto_inc
mattlord Oct 1, 2024
eaed239
Move missing sequence table creation to its own function
mattlord Oct 1, 2024
ec94e1d
Comment nits
mattlord Oct 1, 2024
73f9491
Add --global-keyspace to TestVtctldclientCLI test
mattlord Oct 2, 2024
733198b
Address review comments
mattlord Oct 2, 2024
67c1ef1
Correct comment
mattlord Oct 2, 2024
316a111
Merge remote-tracking branch 'origin/main' into vrepl_replace_auto_inc
mattlord Oct 3, 2024
a56010e
Add section about the new feature to the release summary
mattlord Oct 3, 2024
cd7eea1
Correct flag value
mattlord Oct 3, 2024
14b6873
Add reference table linnk
mattlord Oct 3, 2024
3ceb253
Update summary note
mattlord Oct 4, 2024
2bb2a3a
Merge remote-tracking branch 'origin/main' into vrepl_replace_auto_inc
mattlord Oct 4, 2024
1d8601a
Merge remote-tracking branch 'origin/main' into vrepl_replace_auto_inc
mattlord Oct 4, 2024
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
2 changes: 1 addition & 1 deletion go/cmd/vtctldclient/command/vreplication/common/utils.go
Original file line number Diff line number Diff line change
@@ -287,7 +287,7 @@ func AddCommonSwitchTrafficFlags(cmd *cobra.Command, initializeTargetSequences b
cmd.Flags().BoolVar(&SwitchTrafficOptions.DryRun, "dry-run", false, "Print the actions that would be taken and report any known errors that would have occurred.")
cmd.Flags().BoolVar(&SwitchTrafficOptions.Force, "force", false, "Force the traffic switch even if some potentially non-critical actions cannot be performed; for example the tablet refresh fails on some tablets in the keyspace. WARNING: this should be used with extreme caution and only in emergency situations!")
if initializeTargetSequences {
cmd.Flags().BoolVar(&SwitchTrafficOptions.InitializeTargetSequences, "initialize-target-sequences", false, "When moving tables from an unsharded keyspace to a sharded keyspace, initialize any sequences that are being used on the target when switching writes.")
cmd.Flags().BoolVar(&SwitchTrafficOptions.InitializeTargetSequences, "initialize-target-sequences", false, "When moving tables from an unsharded keyspace to a sharded keyspace, initialize any sequences that are being used on the target when switching writes. If the sequence table is not found AND the sequence table reference was fully qualified OR a value was specified for --global-keyspace, then we will attempt to create each sequence table in that keyspace if it doesn't exist.")
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear exactly what the boolean logic here means to someone who doesn't already know it.
I'm reading it as

(IF the sequence table is not found) AND ((the sequence table reference was fully qualified) OR (a value was specified for --global-keyspace))

but it should be rewritten to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can think about how to make it more clear. It reads pretty clear to me but I'll think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to address this here: 733198b

The change is subtle, but I think it's quite clear.

}
}

15 changes: 15 additions & 0 deletions go/cmd/vtctldclient/command/vreplication/movetables/create.go
Original file line number Diff line number Diff line change
@@ -40,6 +40,8 @@ var (
NoRoutingRules bool
AtomicCopy bool
WorkflowOptions vtctldatapb.WorkflowOptions
// This maps to a WorkflowOptions.StripShardedAutoIncrement ENUM value.
StripShardedAutoIncrementStr string
Copy link
Member

Choose a reason for hiding this comment

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

Along with the proto name change, we should change this name. It is confusing to call it Strip when it could be NONE/REMOVE/REPLACE.

Copy link
Member

Choose a reason for hiding this comment

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

There seems to be some nuance here that I missed during review. So maybe you just need to explain what I'm missing :)
Quoting Rohit:

Maybe consider naming the StripShardedAutoIncrement differently for the string and enum representations. Also adding a comment to note that the first one is from the CLI flags and second to set in the _vt.vreplication row.

Copy link
Contributor Author

@mattlord mattlord Oct 2, 2024

Choose a reason for hiding this comment

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

OK. I'll rename it to ShardedAutoIncrementHandling[Str]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to address this here: 733198b

mattlord marked this conversation as resolved.
Show resolved Hide resolved
}{}

// create makes a MoveTablesCreate gRPC call to a vtctld.
@@ -87,6 +89,19 @@ var (
return fmt.Errorf("cannot specify both --tenant-id (i.e. a multi-tenant migration) and --source-shards (i.e. a shard-by-shard migration)")
}

// createOptions.StripShardedAutoIncrementStr is the CLI flag value provided and
// we need to map that to the ENUM value for createOptions.WorkflowOptions.StripShardedAutoIncrement
// which gets saved in the _vt.vreplication record's options column.
createOptions.StripShardedAutoIncrementStr = strings.ToUpper(createOptions.StripShardedAutoIncrementStr)
val, ok := vtctldatapb.ShardedAutoIncrementHandling_value[createOptions.StripShardedAutoIncrementStr]
if !ok {
return fmt.Errorf("invalid value provided for --strip-sharded-auto-increment, valid values are: %s", stripShardedAutoIncStrOptions)
}
createOptions.WorkflowOptions.StripShardedAutoIncrement = vtctldatapb.ShardedAutoIncrementHandling(val)
if val == int32(vtctldatapb.ShardedAutoIncrementHandling_REPLACE) && createOptions.WorkflowOptions.GlobalKeyspace == "" {
fmt.Println("WARNING: no global-keyspace value provided so all sequence table references not fully qualified must be created manually before switching traffic")
}

return nil
},
RunE: commandCreate,
Original file line number Diff line number Diff line change
@@ -17,10 +17,15 @@ limitations under the License.
package movetables

import (
"fmt"
"strings"

"github.com/spf13/cobra"

"vitess.io/vitess/go/cmd/vtctldclient/command/vreplication/common"
"vitess.io/vitess/go/vt/topo/topoproto"

vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata"
)

var (
@@ -32,6 +37,7 @@ var (
Aliases: []string{"movetables"},
Args: cobra.ExactArgs(1),
}
stripShardedAutoIncStrOptions string
)

func registerCommands(root *cobra.Command) {
@@ -49,8 +55,11 @@ func registerCommands(root *cobra.Command) {
create.Flags().BoolVar(&createOptions.NoRoutingRules, "no-routing-rules", false, "(Advanced) Do not create routing rules while creating the workflow. See the reference documentation for limitations if you use this flag.")
create.Flags().BoolVar(&createOptions.AtomicCopy, "atomic-copy", false, "(EXPERIMENTAL) A single copy phase is run for all tables from the source. Use this, for example, if your source keyspace has tables which use foreign key constraints.")
create.Flags().StringVar(&createOptions.WorkflowOptions.TenantId, "tenant-id", "", "(EXPERIMENTAL: Multi-tenant migrations only) The tenant ID to use for the MoveTables workflow into a multi-tenant keyspace.")
create.Flags().BoolVar(&createOptions.WorkflowOptions.StripShardedAutoIncrement, "remove-sharded-auto-increment", true, "If moving the table(s) to a sharded keyspace, remove any auto_increment clauses when copying the schema to the target as sharded keyspaces should rely on either user/application generated values or Vitess sequences to ensure uniqueness.")
create.Flags().StringSliceVar(&createOptions.WorkflowOptions.Shards, "shards", nil, "(EXPERIMENTAL: Multi-tenant migrations only) Specify that vreplication streams should only be created on this subset of target shards. Warning: you should first ensure that all rows on the source route to the specified subset of target shards using your VIndex of choice or you could lose data during the migration.")
create.Flags().StringVar(&createOptions.WorkflowOptions.GlobalKeyspace, "global-keyspace", "", "If specified, then attempt to create any global resources here such as sequence tables needed to replace auto_increment table clauses that are removed due to --remove-sharded-auto-increment=REPLACE. The value must be an unsharded keyspace that already exists.")
deepthi marked this conversation as resolved.
Show resolved Hide resolved
create.Flags().StringVar(&createOptions.StripShardedAutoIncrementStr, "remove-sharded-auto-increment", vtctldatapb.ShardedAutoIncrementHandling_REMOVE.String(),
fmt.Sprintf("If moving the table(s) to a sharded keyspace, remove any MySQL auto_increment clauses when copying the schema to the target as sharded keyspaces should rely on either user/application generated values or Vitess sequences to ensure uniqueness. If REPLACE is specified then they are automatically replaced by Vitess sequence definitions. (options are: %s)",
stripShardedAutoIncStrOptions))
base.AddCommand(create)

opts := &common.SubCommandsOpts{
@@ -100,4 +109,17 @@ func registerCommands(root *cobra.Command) {

func init() {
common.RegisterCommandHandler("MoveTables", registerCommands)

mattlord marked this conversation as resolved.
Show resolved Hide resolved
sb := strings.Builder{}
strvals := make([]string, len(vtctldatapb.ShardedAutoIncrementHandling_name))
for enumval, strval := range vtctldatapb.ShardedAutoIncrementHandling_name {
strvals[enumval] = strval
}
for i, v := range strvals {
if i > 0 {
sb.WriteByte(',')
}
sb.WriteString(v)
}
stripShardedAutoIncStrOptions = sb.String()
}
Original file line number Diff line number Diff line change
@@ -110,8 +110,8 @@ func testMoveTablesFlags1(t *testing.T, mt *iMoveTables, sourceKeyspace, targetK
createFlags := []string{"--auto-start=false", "--defer-secondary-keys=false", "--stop-after-copy",
"--no-routing-rules", "--on-ddl", "STOP", "--exclude-tables", "customer2",
"--tablet-types", "primary,rdonly", "--tablet-types-in-preference-order=true",
"--all-cells",
"--config-overrides", mapToCSV(overrides),
"--all-cells", "--config-overrides", mapToCSV(overrides),
"--remove-sharded-auto-increment=REPLACE", fmt.Sprintf("--global-keyspace=%s", sourceKeyspace),
}
completeFlags := []string{"--keep-routing-rules", "--keep-data"}
switchFlags := []string{}
@@ -591,6 +591,10 @@ func validateMoveTablesWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow)
require.Equalf(t, 1, len(bls.Filter.Rules), "Rules are %+v", bls.Filter.Rules) // only customer, customer2 should be excluded
require.Equal(t, binlogdatapb.OnDDLAction_STOP, bls.OnDdl)
require.True(t, bls.StopAfterCopy)

// Validate the remove-sharded-auto-increment related value handling.
require.Equal(t, vtctldatapb.ShardedAutoIncrementHandling_REPLACE, vtctldatapb.ShardedAutoIncrementHandling(wf.Options.StripShardedAutoIncrement))
require.Equal(t, wf.Source.Keyspace, wf.Options.GlobalKeyspace)
}

// Test that routing rules can be applied using the vtctldclient CLI for all types of routing rules.
Loading
Loading