Remove Usage of VReplicationExec For _vt.vreplication Reads #14424
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: Matt Lord <[email protected]>
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
NeedsDescriptionUpdate
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
1ce5ab3 to
e9b94d8
Compare
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
2 times, most recently
from
41dcece to
68c41ff
Compare
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
68c41ff to
1045dd5
Compare
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
d470f96 to
8f49c6b
Compare
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
8f49c6b to
fcc631d
Compare
mattlord
added
Type: Internal Cleanup
Component: VReplication
and removed
NeedsIssue
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
github-actions
bot
added
the
Stale
mattlord
removed
the
Stale
mattlord
changed the title
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
bac951f to
7bfcb8b
Compare
mattlord
changed the title
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
7bfcb8b to
4c34f54
Compare
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
4c34f54 to
4e0a542
Compare
rohit-nayak-ps
requested changes
Feb 9, 2024
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
cfddc38 to
471c7a3
Compare
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
rm_vrepl_exec
branch
from
471c7a3 to
9162c6f
Compare
… comments Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
1. Add a new check to the e2e test that confirms all streams are running after a Reshard. 2. Address a TODO on Reshard stream migrations 3. Fix a bug in readRefStreams (was using the workflow struct rather (than workflow name in the map key) 4. Moves more spots to using proto getters as they are nil safe Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
|
@rohit-nayak-ps I've addressed your review comments now and the tests are all green again. Please let me know what you think. Thanks! |
8 tasks
| sourceShards: []string{"-"}, | ||
| targetShards: []string{"-80", "80-"}, | ||
| wantReqs: map[uint32]*tabletmanagerdatapb.CreateVReplicationWorkflowRequest{ | ||
| 200: { |
There was a problem hiding this comment.
The expected requests are quite similar: only shards are different in this and the next, for example. A helper function to get the request based on the differing attributes might be useful. It will both reduce the size of the test and make it easier to see what is the difference in the expectation.
There was a problem hiding this comment.
Can you show me an example of what you mean? I don't see a clear and intuitive way to do this w/o the test case NOT containing the logic or explicitly stating what each test case wants.
There was a problem hiding this comment.
I can do this later when I am refactoring the tests. Let's get this PR merged for now!
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
rohit-nayak-ps
approved these changes
Mar 10, 2024
5 tasks
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 replaces reads (i.e.
SELECTs) against the_vt.vreplicationsidecar table in thevtctldclientimplementation (vtctl/vtctlclientare also deprecated) that were using the deprecatedVReplicationExecRPC with the newertabletmanagerRPCs which do not expose the implementation (SQL against tables) and thus address upgrade/downgrade issues.Note: the deprecated
vtctlclient/wranglerimplementation is left unchanged — so that it can be used as a fallback if there are any issues with the newvtctldclientimplementation until it's eventually removed — by adding legacy functions in its code path as needed.It also begins the process of unifying the creation of VReplication workflow streams in
vtctldclientusing the newtabletmanagerRPCs.MaterializeandReshardwere still using the deprecatedVReplicationExecRPC and here we generalized theMoveTablesusage of the new RPCs and movedMaterializeover to that.Related Issue(s)
Checklist