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
Multi-tenant MoveTables: Create vreplication streams only on specified shards #15746
Multi-tenant MoveTables: Create vreplication streams only on specified shards #15746
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15746 +/- ##
==========================================
- Coverage 68.43% 68.43% -0.01%
==========================================
Files 1558 1558
Lines 196186 196201 +15
==========================================
+ Hits 134269 134270 +1
- Misses 61917 61931 +14 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I only had some minor comments that you can address as you feel best.
go/cmd/vtctldclient/command/vreplication/movetables/movetables.go
Outdated
Show resolved
Hide resolved
@@ -49,6 +49,7 @@ 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) 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, "(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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create.Flags().StringSliceVar(&createOptions.WorkflowOptions.Shards, "shards", nil, "(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().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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this should be called target-shards
given that we already have a source-shards
option. I wonder what happens if someone specifies both of these options. Do we want to prevent that? Or ignore source-shards
if tenant-id
is specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, may as well expand EXPERIMENTAL
in the flag help for tenant-id (line 50) to EXPERIMENTAL: Multi-tenant migrations only
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had started with --target-shards
, but it wasn't uniform with our implementation for shard-by-shard migrations where shard subsets are specified with --shards
.
Added a validation to ensure that both --source-shards
and --tenant-id
cannot be specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unfortunate. I guess --shards
made sense for shard-by-shard because it's very clear which shard you are currently migrating. That is not the case for multi-tenant. What do you think about adding --target-shards
for multi-tenant migrations, and "migrating" shard-by-shard migrations to use that in the next release (deprecate old flag, add new flag, respect both, delete in next release etc.)? Or we can even wait to do that until multi-tenant is no longer experimental. I'll approve anyhow, it's not worth holding up this PR, but I feel the UX will be better and clearer if we call it --target-shards
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of changing just the `create option, but it looks inconsistent while trying to document it since it is different for the other actions. Merging as-is for now.
…set to Create and other commands Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…ied, for uniformity and fixing failing unit test Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…migrations Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
2ed7aaf
to
4a1db5d
Compare
Description
Adds an option to provide
shards
for multi-tenant migrations. This will only create the workflow streams on the specified shard(s). It is expected that the user correctly maps the tenant id to the shard as per the configured vindex.This PR also fixes a couple of places where we had either missed out or regressed on filtering our target shards for all MoveTables commands where shards were specified.
Related Issue(s)
#15748
Checklist
Deployment Notes