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

Vtctl: Adding VExec and Workflow commands #6410

Merged
merged 10 commits into from
Jul 27, 2020

Conversation

rohit-nayak-ps
Copy link
Contributor

No description provided.

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>
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

I've only eyeballed it. Approach looks good for now. I'll do a deeper dive later.

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

One idea I had was to actually have the tablet's VReplicationExec always add the dbname constraint to all the statements. In the case of inserts, it should add the field.

If the constraint is already present, it should verify. This should simplify the vexec code as well as make VReplicationExec more robust.

…onal

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Copy link
Contributor Author

One idea I had was to actually have the tablet's VReplicationExec always add the dbname constraint to all the statements. In the case of inserts, it should add the field.

If the constraint is already present, it should verify. This should simplify the vexec code as well as make VReplicationExec more robust.

Actually vexec is anyway adding the workflow constraint, so currently dbname is being checked/added at the same time as workflow. So as far as I can see the vexec code will not be simplified.

Some of this code will have to be repeated in VReplicationExec if dbname is handled there. Not sure I understand why checking in VReplicationExec makes it more robust: the tablets are anyway chosen by getting the shard masters and we then use the dbname from the provided tabletInfo.

I have missed verifying that any user-specified where constraints match the provided workflow/keyspace. Will add.

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps changed the title WIP: Adding VExec and Workflow commands to vtctl Vtctl: Adding VExec and Workflow commands Jul 16, 2020
@rohit-nayak-ps rohit-nayak-ps marked this pull request as ready for review July 16, 2020 10:15
@rohit-nayak-ps
Copy link
Contributor Author

One idea I had was to actually have the tablet's VReplicationExec always add the dbname constraint to all the statements. In the case of inserts, it should add the field.

If the constraint is already present, it should verify. This should simplify the vexec code as well as make VReplicationExec more robust.

As discussed we will be adding the dbname constraint to VReplicationExec as a followup PR. VExec/Workflow will retain current implementation where they add the dbname constraint.

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Nice work. Some quesitions.

return newWhere
}

func (vx *vexec) buildInsertPlan(ins *sqlparser.Insert) (*vexecPlan, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something we have to watch, and may have to remove. I can't think of a use case where this will make sense yet. Commands like Reshard etc. are better abstractions in some sense.

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 removed this.

I had not implemented it at first, but we decided to add this for completeness. But yes, it looks pretty useless atm: we can implement any special workflows when we see a need.

…ted in review

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@sougou sougou merged commit fa67866 into vitessio:master Jul 27, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
@shlomi-noach shlomi-noach deleted the rn-vtctl-vexec-workflow branch August 10, 2020 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants