Address review comments

Signed-off-by: Matt Lord <mattalord@gmail.com>

go/cmd/vtctldclient/command/vreplication/common/utils.go

@@ -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. 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.")
+		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 the sequence table in that keyspace.")
 	}
 }
 

go/cmd/vtctldclient/command/vreplication/movetables/create.go

@@ -40,8 +40,8 @@ var (
 		NoRoutingRules      bool
 		AtomicCopy          bool
 		WorkflowOptions     vtctldatapb.WorkflowOptions
-		// This maps to a WorkflowOptions.StripShardedAutoIncrement ENUM value.
-		StripShardedAutoIncrementStr string
+		// This maps to a WorkflowOptions.ShardedAutoIncrementHandling ENUM value.
+		ShardedAutoIncrementHandlingStr string
 	}{}
 
 	// create makes a MoveTablesCreate gRPC call to a vtctld.
@@ -89,15 +89,16 @@ 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]
+			// createOptions.ShardedAutoIncrementHandlingStr is the CLI flag value
+			// provided and we need to map that to the ENUM value for
+			// createOptions.WorkflowOptions.ShardedAutoIncrementHandling which
+			// gets saved in the _vt.vreplication record's options column.
+			createOptions.ShardedAutoIncrementHandlingStr = strings.ToUpper(createOptions.ShardedAutoIncrementHandlingStr)
+			val, ok := vtctldatapb.ShardedAutoIncrementHandling_value[createOptions.ShardedAutoIncrementHandlingStr]
 			if !ok {
-				return fmt.Errorf("invalid value provided for --strip-sharded-auto-increment, valid values are: %s", stripShardedAutoIncStrOptions)
+				return fmt.Errorf("invalid value provided for --sharded-auto-increment-handling, valid values are: %s", shardedAutoIncHandlingStrOptions)
 			}
-			createOptions.WorkflowOptions.StripShardedAutoIncrement = vtctldatapb.ShardedAutoIncrementHandling(val)
+			createOptions.WorkflowOptions.ShardedAutoIncrementHandling = 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")
 			}

go/cmd/vtctldclient/command/vreplication/movetables/movetables.go

@@ -37,7 +37,7 @@ var (
 		Aliases:               []string{"movetables"},
 		Args:                  cobra.ExactArgs(1),
 	}
-	stripShardedAutoIncStrOptions string
+	shardedAutoIncHandlingStrOptions string
 )
 
 func registerCommands(root *cobra.Command) {
@@ -56,10 +56,15 @@ func registerCommands(root *cobra.Command) {
 	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().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.")
-	create.Flags().StringVar(&createOptions.StripShardedAutoIncrementStr, "remove-sharded-auto-increment", vtctldatapb.ShardedAutoIncrementHandling_REMOVE.String(),
+	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 --sharded-auto-increment-handling=REPLACE. The value must be an unsharded keyspace that already exists.")
+	create.Flags().StringVar(&createOptions.ShardedAutoIncrementHandlingStr, "sharded-auto-increment-handling", 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))
+			shardedAutoIncHandlingStrOptions))
+	// This flag was deprecated in v21 so can be removed in v21+.
+	create.Flags().StringVar(&createOptions.ShardedAutoIncrementHandlingStr, "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)",
+			shardedAutoIncHandlingStrOptions))
+	create.Flags().MarkDeprecated("remove-sharded-auto-increment", "please use --sharded-auto-increment-handling instead.")
 	base.AddCommand(create)
 
 	opts := &common.SubCommandsOpts{
@@ -121,5 +126,5 @@ func init() {
 		}
 		sb.WriteString(v)
 	}
-	stripShardedAutoIncStrOptions = sb.String()
+	shardedAutoIncHandlingStrOptions = sb.String()
 }

go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go

@@ -111,7 +111,7 @@ func testMoveTablesFlags1(t *testing.T, mt *iMoveTables, sourceKeyspace, targetK
 		"--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),
-		"--remove-sharded-auto-increment=REPLACE", fmt.Sprintf("--global-keyspace=%s", sourceKeyspace),
+		"--sharded-auto-increment-handling=REPLACE", fmt.Sprintf("--global-keyspace=%s", sourceKeyspace),
 	}
 	completeFlags := []string{"--keep-routing-rules", "--keep-data"}
 	switchFlags := []string{}
@@ -592,8 +592,8 @@ func validateMoveTablesWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow)
 	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))
+	// Validate the sharded-auto-increment-handling related value handling.
+	require.Equal(t, vtctldatapb.ShardedAutoIncrementHandling_REPLACE, vtctldatapb.ShardedAutoIncrementHandling(wf.Options.ShardedAutoIncrementHandling))
 	require.Equal(t, wf.Source.Keyspace, wf.Options.GlobalKeyspace)
 }