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

OnlineDDL: request_context/migration_context #7082

Merged
merged 3 commits into from
Dec 3, 2020

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Nov 29, 2020

We add a context to an OnlineDDL request:

  • who made the request? (e.g. vtctl, vtctld/api, vtgate)
  • context parameters? (e.g. session ID, or unique ID for vtctl execution)

This context (not to confuse with golang context.Context) can be later used to group and associate migrations. For example, knowing that a few migration requests all came from the same vtctl call, means we should probably prioritize to run them together.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Still TODO: pass request_by and request_param in VExec query. I struggle to figure this one out, I think we're shooting ourselves in the foot with VExec: how do you add new columns to a Vexec query, and at the same time make it backwards compatible? Specifically, if a user upgrades their vtctld or vtgate servers, but not their vttablet servers, then the addition of new columns is rejected by vttablet. VExec creates coupling where normal golang mechanisms already provide the decoupling framework to deal with missing/default values.

@sougou
Copy link
Contributor

sougou commented Nov 29, 2020

Since VExec is not GA yet, it doesn't have to be backward compatible.
For grouping: we've been using unique workflow names for vreplication, and that has worked out well so far. We can see if the same approach works here.
As for prioritization, it probably needs to be enforced by a policy because different people may want different things.

…t-context

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach changed the title OnlineDDL: operation context: request_by and request_param OnlineDDL: request_context/migration_context Dec 3, 2020
@shlomi-noach
Copy link
Contributor Author

Converged into a single param, migration_context or request_context, that is an arbitrary string.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Because Online DDL is still experimental, and because I need this functionality for some internal POC, I'm just modifying the schema without beackwards compatibility.

@shlomi-noach
Copy link
Contributor Author

ready for review

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi deepthi merged commit 84f49b2 into vitessio:master Dec 3, 2020
@deepthi deepthi deleted the online-ddl-request-context branch December 3, 2020 21:15
@askdba askdba added this to the v9.0 milestone Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants