Online DDL: Fail a --in-order-completion migration, if a _prior_ migration within the same context is 'failed' or 'cancelled' #16071
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
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
…ly bail out when one migration fails. This last test will fail and a followup commit will implement the code Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
… cancelled/failed Signed-off-by: Shlomi Noach <[email protected]>
…n' query picks a failed or cancelled migration with a given migration context, and which comes before a given migration Signed-off-by: Shlomi Noach <[email protected]>
… the same context is 'failed' or 'cancelled'. Failing happens either at 'ready' state or at 'running' state Signed-off-by: Shlomi Noach <[email protected]>
shlomi-noach
added
Type: Bug
Component: Online DDL
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
|
vitess-bot
bot
added
NeedsBackportReason
shlomi-noach
removed
NeedsDescriptionUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16071 +/- ##
==========================================
- Coverage 68.20% 68.20% -0.01%
==========================================
Files 1541 1541
Lines 197332 197369 +37
==========================================
+ Hits 134598 134619 +21
- Misses 62734 62750 +16 ☔ View full report in Codecov by Sentry. |
mattlord
approved these changes
Jun 5, 2024
| t.Run("drop multiple tables and views, in-order-completion", func(t *testing.T) { | ||
| uuidList := testOnlineDDLStatement(t, createParams(sql, ddlStrategy+" --in-order-completion", "vtctl", "", "", true)) // skip wait | ||
| vuuids = strings.Split(uuidList, "\n") | ||
| assert.Equal(t, 4, len(vuuids)) |
There was a problem hiding this comment.
If you didn't already know, you can use assert.Len too/instead.
There was a problem hiding this comment.
I did not know!
Signed-off-by: Shlomi Noach <[email protected]>
shlomi-noach
commented
Jun 6, 2024
Comment on lines
+3462
to
+3472
| // We will fail an in-order migration if there's _prior_ migrations within the same migration-context | ||
| // which have failed. | ||
| if onlineDDL.StrategySetting().IsInOrderCompletion() { | ||
| wasFailed, err := e.validateInOrderMigration(ctx, onlineDDL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if wasFailed { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Most of this function is just an extract/refactor out of runNextMigration() and into an independent function of its own. But these lines above are then also added to fail a migration before even running it, if the in-order terms are not met.
dbussink
approved these changes
Jun 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Fixes #16070
Either at scheduling time, or when
running, for a given--in-order-completionmigration, if we find another migration in the samemigration-contextthat isfailedorcancelled, and that other migration precedes our given migration chronologically (has a loweridvalue inschema_migrationstable), then we fail our given migration.Basically the idea is: if 5 migrations are meant to run in-order, and 3rd migration happens to fail, then 4th and 5th migrations should fail as well and should not be allowed to complete. If they were allowed to complete, they'd run out of order.
Related Issue(s)
#16070
Checklist
Deployment Notes