-
Notifications
You must be signed in to change notification settings - Fork 2k
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
VReplication: Improve workflow cancel/delete #15977
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
9fd6793
to
56bcebb
Compare
Otherwise the cancel/delete and cleanup work can fail. Signed-off-by: Matt Lord <mattalord@gmail.com>
56bcebb
to
0b12d82
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
b2d3777
to
d2d5efe
Compare
d2d5efe
to
d7b39d7
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
d7b39d7
to
82005fb
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
1ddd859
to
7dd69c2
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
These changes did not end up being necessary. Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
rohit-nayak-ps
approved these changes
Jun 12, 2024
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.
Thanks! Great to see the unit test framework enhancements.
harshit-gangal
approved these changes
Jun 12, 2024
vitess-bot
pushed a commit
that referenced
this pull request
Jun 12, 2024
Signed-off-by: Matt Lord <mattalord@gmail.com>
vitess-bot
pushed a commit
that referenced
this pull request
Jun 12, 2024
Signed-off-by: Matt Lord <mattalord@gmail.com>
frouioui
pushed a commit
that referenced
this pull request
Jun 12, 2024
… (#16130) Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
mattlord
pushed a commit
that referenced
this pull request
Jun 12, 2024
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Backport to: release-20.0-rc
Needs to be backport to release-20.0-rc
Backport to: release-20.0
Needs to be backport to release-20.0
Component: VReplication
Type: Bug
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This work makes three key changes:
Workflow Delete
(this is used by thecancel
sub-command in all of the workflows:MoveTables
/Reshard
, etc). Otherwise you can see unexpected query errors in the workflow against the table between when the table is dropped and the workflow deleted.- This is otherwise benign, but we should still do these operations in a correct order
- If you're doing a
ReverseTraffic
call where the original (now frozen) workflow is deleted and another variant of it created, then this could result in the workflow being gone/deleted- Related to this, we now put those
DeniedTables
entries — a secondary safety measure, in addition to the routing rules, which prevent writes from being executed against the wrong side/keyspace which would result in data drift and potentially lost writes (if not otherwise detected with VDiff and manually corrected) — in place on the target during workflow creation rather than waiting until the first traffic switch. Previously those would only be put in place on the original workflow target afterSwitchTraffic
->ReverseTraffic
.- More generally, we now do not fail the delete/cancel if the thing we went to remove is already gone or the thing we went to add is already in place
MoveTables
workflows, we should always haveDeniedTables
entries on the target keyspace so I standardized the handling of those when doing traffic switchesFinally, I added a new unit test framework and tests to cover this new behavior and moved the basic VReplication endtoend test over to explicitly/statically using
vtctldclient
where it was previously explicitly/statically usingvtctlclient
(the V2 test usesvtctldclient
exclusively whereas the basic test chooses the client randomly for most things so that we continue testing both untilvtctlclient
is removed — the things I changed are the instances where we were using a static client).Related Issue(s)
Checklist