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.

Sign up for GitHub

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

Jump to bottom

v15 backport: Onlineddl: formalize "immediate operations", respect --postpone-completion strategy flag #13832

Merged
merged 5 commits into from
Aug 28, 2023
Merged

v15 backport: Onlineddl: formalize "immediate operations", respect --postpone-completion strategy flag #13832

merged 5 commits into from
Aug 28, 2023

Conversation

shlomi-noach
Copy link
Contributor

This is a backport of #11910 into v15

#11910 was merged Dec 2022, but still did not make it to v15. This bugfix is important and since we still support v15 it should be backported.

Description

This PR formalizes what "immediate operations" are:

Immediate operations are ones that can be performed within a split second, or rather, do not require long
running processes. Immediate operations are:

  • CREATE TABLE
  • DROP TABLE (which we convert into RENAME)
  • All VIEW operations
  • An INSTANT DDL accompanied by relevant ddl strategy flags

Non immediate operations are:

  • A gh-ost migration
  • A vitess (vreplication) migration

In #11898 we find that ALTER VIEW and instant DDLs do not respect --postpone-completion. This PR solves that for both, and all other, cases. Any "immediate operation" now respects that flag, including REVERTs of such operations.

endtoend tests added to cover cases.

Related Issue(s)

Fixes #11898
This PR supersedes and replaces #11899
#6926

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@shlomi-noach @deepthi
Onlineddl: formalize "immediate operations", respect `--postpone-comp…
24b9903 
…letion` strategy flag (vitessio#11910)

* Added endtoend test to validate postponed REVERT of an ALTER VIEW statement: test indicates a bug

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

* ALTER VIEW now respects --postpone-completion flag

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

* Online DDL: formalize 'immediate operations', respect --postpone-completion

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

* typo

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

* only test instant ddl in capable versions (ie 8.0)

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

* more specific instant ddl capability

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

* Update go/vt/vttablet/onlineddl/executor.go

Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* Update go/vt/vttablet/onlineddl/executor.go

Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* Update go/vt/vttablet/onlineddl/executor.go

Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com>
@shlomi-noach shlomi-noach added Type: Bug Backport This is a backport Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) labels Aug 22, 2023
@shlomi-noach shlomi-noach requested a review from a team August 22, 2023 10:57
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Aug 22, 2023 •
edited by ajm188
Loading

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Aug 22, 2023
@shlomi-noach shlomi-noach marked this pull request as ready for review August 22, 2023 10:57
@github-actions github-actions bot added this to the v15.0.5 milestone Aug 22, 2023
@shlomi-noach shlomi-noach removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Aug 23, 2023
@shlomi-noach
Copy link
Contributor Author

Hmmm seems like onlineddl_scheduler is failing, and this is related to the backport. Looking into.

ajm188
ajm188 approved these changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

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

approved!

go/vt/vttablet/onlineddl/executor.go Outdated Show resolved Hide resolved
@shlomi-noach @ajm188
Update go/vt/vttablet/onlineddl/executor.go
a56d230 
Co-authored-by: Andrew Mason <amason@hey.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@rohit-nayak-ps
Copy link
Contributor

vtgate's not starting in Cluster (onlineddl_vrepl_stress_suite): maybe a new flag specified in the test, but not in v15?

2023-08-24T11:10:25.9859564Z I0824 11:10:25.985266   15224 vtgate_process.go:131] Running vtgate with command: vtgate --topo_implementation etcd2 --topo_global_server_address localhost:16002 --topo_global_root /vitess/global --log_dir /tmp/vt_3264845765/vtroot_16001/tmp_16003 --log_queries_to_file /tmp/vt_3264845765/vtroot_16001/tmp_16003/vtgate_querylog.txt --port 16010 --grpc_port 16011 --mysql_server_port 16012 --mysql_server_socket_path /tmp/vt_3264845765/vtroot_16001/tmp_16003/mysql.sock --cell zone1 --cells_to_watch zone1 --tablet_types_to_wait PRIMARY,REPLICA --service_map grpc-tabletmanager,grpc-throttler,grpc-queryservice,grpc-updatestream,grpc-vtctl,grpc-vtgateservice --mysql_auth_server_impl none --mysql_server_version 8.0.34-vitess --planner-version Gen4CompareV3 --ddl_strategy online
2023-08-24T11:10:58.8258837Z process 'vtgate' exited prematurely (err: exit status 1)

@shlomi-noach
Removing a specific failing test from the backport. The removed test …
a8cccdb 
…tests '--prefer-instant-ddl', a functionality that was not backported to v15

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

vtgate's not starting in Cluster (onlineddl_vrepl_stress_suite): maybe a new flag specified in the test, but not in v15?

onlineddl_vrepl_stress_suite seems to pass for both 5.7 and 8.0 though?

I meanwhile pushed a fix to the failing onlineddl_scheduler test (skipping the test, confirmed to be related to sunctionality that does not exist in v15). Let's see how the rebuild go.

@shlomi-noach
Copy link
Contributor Author

Yep, getting that onlineddl_vrepl_stress_suite error now.

@shlomi-noach
Copy link
Contributor Author

There's nothing in this backport to affect flags, and really nothing to affect vtgate at all.

@shlomi-noach
Copy link
Contributor Author

looks like unrelated flakiness. Tests passing now.

@shlomi-noach shlomi-noach added Component: General Changes throughout the code base and removed Component: General Changes throughout the code base labels Aug 24, 2023
@shlomi-noach shlomi-noach added the Component: General Changes throughout the code base label Aug 25, 2023
@shlomi-noach
empty commit to kick ci
397eed2 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach removed the Backport This is a backport label Aug 27, 2023
@shlomi-noach
Copy link
Contributor Author

I can't get Check Pull Request labels to run 😮

Check Pull Request labels Expected — Waiting for status to be reported
Screenshot 2023-08-27 at 13 08 47

@shlomi-noach shlomi-noach added the Backport This is a backport label Aug 27, 2023
@shlomi-noach
empty commit to kick ci
d569e70 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach mentioned this pull request Aug 28, 2023
Closed
@shlomi-noach shlomi-noach removed the Component: General Changes throughout the code base label Aug 28, 2023
@shlomi-noach shlomi-noach merged commit a1e0045 into vitessio:release-15.0 Aug 28, 2023
115 checks passed
@shlomi-noach shlomi-noach deleted the onlineddl-immediate-operations-v15 branch August 28, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frouioui frouioui frouioui approved these changes

@ajm188 ajm188 ajm188 approved these changes

@mattlord mattlord Awaiting requested review from mattlord mattlord is a code owner

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps rohit-nayak-ps is a code owner

Assignees
No one assigned
Labels
Backport This is a backport Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) Type: Bug
Projects
None yet
Milestone
v15.0.5
Development

Successfully merging this pull request may close these issues.
4 participants
@shlomi-noach @rohit-nayak-ps @ajm188 @frouioui