[reparentutil / wrangler] Extract PlannedReparentShard logic from wrangler to PlannedReparenter struct #7501
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
ajm188
requested review from
deepthi,
doeg,
setassociative and
rohit-nayak-ps
… PRS This will eventually replace the equivalent logic in wrangler, which can then delegate into this module, along with `VtctldServer` when we add PRS to that API. Signed-off-by: Andrew Mason <amason@slack-corp.com>
This allows conveniently setting up a shard with multiple tablets claiming to be MASTER, with the shard be correctly configured to have _a_ serving master. Previously, those were mutually-exclusive setups with `AddTablets`. Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188
force-pushed
the
am_planned_reparenter
branch
from
1395f55
to
7d53d36
Compare
|
I can review this from a Go perspective but, as with #7464, I defer to experts on the Vitess-y parts. @deepthi, @setassociative, and/or @rohit-nayak-ps would you be able to give this one a look too? |
|
Test failure is coming from a PRS endtoend test, but looks like a port reservation issue. Going to kick it again and also try to repro locally |
ajm188
force-pushed
the
am_planned_reparenter
branch
from
7d53d36
to
7ee9c7a
Compare
ajm188
force-pushed
the
am_planned_reparenter
branch
2 times, most recently
from
1016d29
to
02040df
Compare
doeg
approved these changes
Feb 18, 2021
There was a problem hiding this comment.
This change seems straightforward and looks good to me! (Apart from the failing test, which... seems unrelated? Ugh.) I do suggest waiting for another +1 from one of the other reviewers, though. :)
Truly a heroic amount of faking + testing. 🏆 Seriously good work.
deepthi
approved these changes
Feb 18, 2021
| ) | ||
| } | ||
|
|
||
| // prelightChecks checks some invariants that pr.reparentShardLocked() depends |
There was a problem hiding this comment.
"prelight" 😂
nice catch, should be fixed, and i've cleaned up the commit history as well now
- add preflightChecks tests - add test cases for `performPartialPromotionRecovery` - remove unused arg, add test cases for `performPotentialPromotion` - add missing call to waitgroup add - add test cases for `reparentTablets` - add tests for graceful promotion - add the rest of the PlannedReparenter tests Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188
force-pushed
the
am_planned_reparenter
branch
from
02040df
to
f6913d4
Compare
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 extracts the PRS logic from
./go/vt/wranglerto a dedicatedPlannedReparenterstruct in./go/vt/vtctl/reparentutil, to be shared between the legacy and new vtctl APIs in my next PR, similar to what I did for ERS in #7464.Some notes on commits
All the commits that begin with "wip" are just adding tests, and it's how I committed changes as I went, so it may be useful to step through that way, but before merging I'd like to squash all of those — and just those ones — into a single, "Add all the tests" commit.
Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: