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: implementing -postpone-completion, ALTER VITESS_MIGRATION ... COMPLETE #9171

Merged
merged 14 commits into from
Nov 21, 2021

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Nov 9, 2021

This PR implements:

-postpone-completion

-postpone-completion flag in ddl_strategy. This flag indicates that the migration should not auto-complete. This applies for:

  • any CREATE TABLE
  • any DROP TABLE
  • ALTER table in online strategy
  • ALTER table in gh-ost strategy

Note that this flag is not supported for pt-osc strategy.

Behavior of migrations with this flag:

  • an ALTER table begins, runs, but does not cut-over.
  • CREATE or DROP migrations are silently not even scheduled

Complimenting this flag is:

ALTER VITESS_MIGRATION '-uuid-' COMPLETE

This command indicates that a migration executed with -postpone-completion is good to complete. Behavior:

  • For running ALTERs (online and gh-ost) which are ready to cut-over: cut-over imminently (though not immediately - cut-over depends on polling interval, replication lag, etc)
  • For running ALTERs (online and gh-ost) which are only partly through the migration: they will cut-over automatically when they complete their work, as if -postpone-completion wasn't indicated
  • For queued CREATE and DROP migrations: "unblock" them from being scheduled. They'll be scheduled at the schedulers discretion. there is no guarantee that they will be scheduled to run immediately.

tests added:

  • vrepl endtoend
  • gh-ost endtoend

This PR extends #9160 (not yet merged at this time)
Tracking issue: #6926

…f cut-over for 'online' migrations until value is cleared. ALTER VITESS_MIGRATION ... COMPLETE now implemented and clears the flag. tests added.

Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) labels Nov 9, 2021
Signed-off-by: Shlomi Noach <[email protected]>
…eue posponed migrations. New test fails in th emeantime

Signed-off-by: Shlomi Noach <[email protected]>
…ction fields while queued

Signed-off-by: Shlomi Noach <[email protected]>
…anity check at end of switch

Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach shlomi-noach mentioned this pull request Nov 11, 2021
3 tasks
@shlomi-noach shlomi-noach requested a review from sougou November 15, 2021 13:11
@shlomi-noach
Copy link
Contributor Author

As #9160 is merged, this PR is now ready to review. /cc @deepthi @sougou

@shlomi-noach shlomi-noach marked this pull request as ready for review November 17, 2021 08:47
Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

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

I do not have a lot of context on OnlineDDL, but the changes look good to me. We should wait for someone with more context to review this too :)

go/vt/vttablet/onlineddl/executor.go Outdated Show resolved Hide resolved
Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach
Copy link
Contributor Author

Noting that -postpone-completion implementation is incomplete, as it will withhold -declarative migrations; this will be addressed in a separate PR.

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.

A couple of documentation nits, otherwise LGTM.
It's a pleasure to review a PR that is more tests than code 💯

```sql
alter vitess_migration '9748c3b7_7fdb_11eb_ac2c_f875a4d24e90' cleanup
```
This query tells Vitess that a migration's artifacts are good to be cleaned up asap. This allows Vitess to free disk resources sooner. As a reminder, once a migration's artifacts are cleaned up, the migration is no

This query tells Vitess that a migration's artifacts are good to be cleaned up asap. This allows Vitess to free disk resources sooner. As reminder, once a migration's artifacts are cleaned up, the migration is no
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This query tells Vitess that a migration's artifacts are good to be cleaned up asap. This allows Vitess to free disk resources sooner. As reminder, once a migration's artifacts are cleaned up, the migration is no
This query tells Vitess that a migration's artifacts are good to be cleaned up asap. This allows Vitess to free disk resources sooner. As a reminder, once a migration's artifacts are cleaned up, the migration is no

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


- For running `ALTER`s (`online` and `gh-ost`) which are ready to cut-over: cut-over imminently (though not immediately - cut-over depends on polling interval, replication lag, etc)
- For running `ALTER`s (`online` and `gh-ost`) which are only partly through the migration: they will cut-over automatically when they complete their work, as if `-postpone-completion` wasn't indicated
- For queued `CREATE` and `DROP` migrations: "unblock" them from being scheduled. They'll be scheduled at the schedulers discretion. there is no guarantee that they will be scheduled to run immediately.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- For queued `CREATE` and `DROP` migrations: "unblock" them from being scheduled. They'll be scheduled at the schedulers discretion. there is no guarantee that they will be scheduled to run immediately.
- For queued `CREATE` and `DROP` migrations: "unblock" them from being scheduled. They'll be scheduled at the scheduler's discretion. there is no guarantee that they will be scheduled to run immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Shlomi Noach <[email protected]>
@shlomi-noach
Copy link
Contributor Author

Dcoumentation PR: vitessio/website#887

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants