OnlineDDL: better scheduling/cancellation logic #8603
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
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
|
As it turns out, this improvement to the scheduler also unlocked the would-be next step: automatic recoveries for NativeDDL. This seems to work now! You may begin a NativeDDL (OnlineDDL/VReplication) miration, promote a new primary halfway through, and migration will auto-recover, resume and complete on the new primary.
|
deepthi
reviewed
Aug 10, 2021
…her tablet, updating to its own tablet Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
|
PRS tests are WIP. Meanwhile turning this PR into draft |
Signed-off-by: Shlomi Noach <[email protected]>
|
Added
This... works! Ready for review |
deepthi
approved these changes
Aug 16, 2021
go/vt/vttablet/onlineddl/executor.go
Outdated
| @@ -1329,7 +1336,12 @@ func (e *Executor) readPendingMigrationsUUIDs(ctx context.Context) (uuids []stri | |||
| } | |||
|
|
|||
| // terminateMigration attempts to interrupt and hard-stop a running migration | |||
| func (e *Executor) terminateMigration(ctx context.Context, onlineDDL *schema.OnlineDDL, lastMigrationUUID string) (foundRunning bool, err error) { | |||
| func (e *Executor) terminateMigration(ctx context.Context, onlineDDL *schema.OnlineDDL) (foundRunning bool, err error) { | |||
| // It's possible the killing the migratoin fails for whatever reason, in which case | |||
There was a problem hiding this comment.
Suggested change
| // It's possible the killing the migratoin fails for whatever reason, in which case | |
| // It's possible the killing the migration fails for whatever reason, in which case |
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
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
This PR improves the logic around tracking running migrations and cancelling of stale/broken migrations.
Previously, we tracked:
gh-ostexecuted migration, andpt-oscexecuted migration, andvreplexecuted migrationThis led to some spaghetti code because
vrepldoesn't behave the same way asgh-ostandpt-osc: it doesn't start and end in the same function. It actually doesn't have to start and end in the same tablet.This PR introduces
ownedRunningMigrations: a map whose keys are UUID, which indicate "what migrations this executor expects to be running right now and is happy to own", irrespective of the strategy. In the new logic:vreplthat is found to be running (this is in preparation for better handling of PRS/ERS)This PR also produces better error messages around cancelled migrations. What used to be
"auto cancel"is now a reasoned justified report.We also cleanup some boiletplate code: previously
CancelMigrationaccepted aboolflag indicating whether a running migration should be force-terminated. We find that we always passtruefor this flag, so we remove the flag and just always terminate the migration if found to be running.Existing test suite covers the logic reasonably well. Last week we had a production issue where a couple migrations were
auto cancelled without good reason. At the very least this PR will give us more information, but I believe it will also solve the issue. I'm not sure how to reproduce that production issue, so this is just to confess some cases must elude the existing tests.Related Issue(s)
#6926
Checklist
Deployment Notes