Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[workflow] extract migration targets from wrangler #7934

Merged
merged 18 commits into from
Apr 27, 2021
Merged

[workflow] extract migration targets from wrangler #7934

merged 18 commits into from
Apr 27, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Apr 22, 2021

Description

This is the first in a series of PRs to make VDiff runnable from the new grpcvtctldserver (in order to do that we eventually need the vdiff struct to be defined outside of package wrangler so that both wrangler and grpcvtctldserver can use it, so that's where this is headed).

To start, I extracted buildTargets as a publicly exported function, and then updated all callsites to work on the new types. These should be functionally equivalent.

In the next PR, I'll be looking at moving parts of trafficSwitcher struct to package workflow

Related Issue(s)

Checklist

  • Should this PR be backported? No
  • Tests were added or are not required (other tests should be covered by other wrangler/vrep tests)
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@ajm188 ajm188 requested a review from deepthi as a code owner April 22, 2021 17:15
@ajm188 ajm188 changed the title Am extract migration targets from wrangler [workflow] extract migration targets from wrangler Apr 22, 2021
@ajm188
Copy link
Contributor Author

ajm188 commented Apr 22, 2021

Going to set this back to draft while I take time to look into the tests. I thought it was just a query formatting issue, but there might be something else going on.

@ajm188 ajm188 marked this pull request as draft April 22, 2021 19:18
@ajm188
Copy link
Contributor Author

ajm188 commented Apr 22, 2021

oooookay that was fun. re-opening!!

@ajm188 ajm188 marked this pull request as ready for review April 22, 2021 20:31
@ajm188 ajm188 force-pushed the am_extract_migration_targets_from_wrangler branch from 2689685 to df30470 Compare April 22, 2021 20:32
rafael
rafael approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@rafael rafael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really straightforward from my perspective and makes sense. I think we should get @rohit-nayak-ps eyes just to make sure we get his input on this approach as well.

@ajm188
Copy link
Contributor Author

ajm188 commented Apr 24, 2021

Organizational note: I'm planning on squashing this one. The commits are mainly there to break up the diff into reasonably-small chunks but this change really only makes sense logistically as one atomic commit.

@ajm188
Copy link
Contributor Author

ajm188 commented Apr 26, 2021

I'm going to rebase on master to try to get that tabletmanager_tablegc check to actually start (there's not even a button for me to retrigger it from what i can tell)

@ajm188
Sort imports
3574059 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
WIP: add new workflow.TrafficSwitcher
15fadc1 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Reimplement code in traffic_switcher.go to use new Migration{Source,T…
23524bd 
…arget} types

Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Sort imports
64ead23 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Update types in switcher_dry_run.go
b074f7f 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Sort imports
2b1435e 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Update types in vdiff.go
ae30e2f 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Sort imports
6245520 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Update types in stream_migrator.go
af8d0b6 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Sort imports
de4becd 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Update types in workflow.go
57e0989 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Remove old types from package wrangler
771689b 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Add documentation to new traffic_switcher file
daaf136 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
fixup! WIP: add new workflow.TrafficSwitcher
48cc3cf 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Update remaining calls to reverseWorkflow and move tests over to pa…
5e620db 
…ckage, delete old func

Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Copy over encodeString
960a547 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
[debug] test fixes
732e839 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188
Extend error checking in getWorkflowState to be less sensitive to e…
55f8aab 
…xact error strings

Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188 ajm188 force-pushed the am_extract_migration_targets_from_wrangler branch from df30470 to 55f8aab Compare April 26, 2021 15:44
rohit-nayak-ps
rohit-nayak-ps approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
nice set of refactors

go/vt/vtctl/workflow/traffic_switcher.go
// NewMigrationSource returns a MigrationSource for the given shard and primary.
//
// (TODO|@ajm188): do we always want to start with (position:"", journaled:false)?
func NewMigrationSource(si *topo.ShardInfo, primary *topo.TabletInfo) *MigrationSource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there is no corresponding NewMigrationTarget() ... Is it because it only used in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there has to be some temporarily public functions so that I can move things piecemeal (I think this is going to be temporary anyway, and if not I can come back and add the corresponding NewMigrationTarget!)

@ajm188 ajm188 merged commit 31d84d6 into vitessio:master Apr 27, 2021
@ajm188 ajm188 added this to In progress in Vtctld Service via automation May 7, 2021
@ajm188 ajm188 moved this from In progress to Done in Vtctld Service May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafael rafael rafael approved these changes

@rohit-nayak-ps rohit-nayak-ps rohit-nayak-ps approved these changes

@doeg doeg Awaiting requested review from doeg

@makinje16 makinje16 Awaiting requested review from makinje16

@deepthi deepthi Awaiting requested review from deepthi

Assignees
No one assigned
Labels
Component: VReplication Type: Internal Cleanup
Projects
Vtctld Service
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ajm188 @rafael @rohit-nayak-ps