-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
MoveTables: update vschema while moving tables with autoincrement from sharded to unsharded #9288
MoveTables: update vschema while moving tables with autoincrement from sharded to unsharded #9288
Conversation
… to unsharded keyspace Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
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 a nit about test flakiness and a couple of questions but nothing blocking IMO. Thanks!
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.
Matt's review looks good. You may merge once it is addressed.
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
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.
Nice work! I had two minor nits/questions, but you can ignore them. Thanks!
done = true | ||
log.Infof("Workflow %s has started", ksWorkflow) | ||
} | ||
case <-time.After(5 * time.Second): |
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.
Minor nit, any reason to have the timeout this low? We risk causing the entire action/test to fail. I'm not sure more time is really needed, just curious. We could also make the wait time a parameter.
Feel free to ignore.
} | ||
output, err := vc.VtctlClient.ExecuteCommandWithOutput("Workflow", ksWorkflow, "show") | ||
require.NoError(t, err) | ||
if strings.Contains(output, "\"State\": \"Running\"") { |
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.
Minor nit, maybe better to use a RegEx or JSON parsing so that we are checking for what we really care about while resilient to whitespace and other minor format changes? Full JSON parsing is probably overkill, maybe RegEx is too :-)
Feel free to ignore.
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
d618519
to
dd042cf
Compare
Description
When a sharded table with autoincrement is moved using MoveTables to an unsharded keyspace, currently the related sequence information is not carried over into the target (unsharded) keyspace's vschema.
This PR fixes that by copying over the sequence info from the source schema for that table. This is verified by adding this use-case to the v2 end-to-end test.
Signed-off-by: Rohit Nayak rohit@planetscale.com
Related Issue(s)
#9002
Checklist