[workflow] Migrate getCellsWith{Shard,Table}ReadsSwitched, TrafficSwitchDirection and TableRemovalType to package workflow
#8190
Conversation
go/vt/vtctl/workflow/server.go
Outdated
| ctx context.Context, | ||
| keyspace string, | ||
| si *topo.ShardInfo, | ||
| ttype topodatapb.TabletType, |
There was a problem hiding this comment.
nit - I think across the codebase we use tabletType for this variable.
go/vt/wrangler/traffic_switcher.go
Outdated
| @@ -156,107 +127,27 @@ func (ts *trafficSwitcher) ForAllTargets(f func(source *workflow.MigrationTarget | |||
| func (wr *Wrangler) getCellsWithShardReadsSwitched(ctx context.Context, targetKeyspace string, si *topo.ShardInfo, tabletType string) ( | |||
| cellsSwitched, cellsNotSwitched []string, err error) { | |||
|
|
|||
| cells, err := wr.ts.GetCellInfoNames(ctx) | |||
| ttype, err := topoproto.ParseTabletType(tabletType) | |||
There was a problem hiding this comment.
If you follow my suggestion from above, here we can:
- Rename to tabletType to be tabletTypeStr
- Just this variable stays
ttype.
go/vt/vtctl/workflow/server.go
Outdated
| continue | ||
| } | ||
|
|
||
| // If reads and writes are both switched it is passible that the |
| // shards are not yet serving, or once reads and writes are both | ||
| // switched. | ||
| if len(partition.GetShardTabletControls()) == 0 { | ||
| noControls = true |
There was a problem hiding this comment.
Good catch on the noControls thing. Let's check with @sougou / @rohit-nayak-ps . I think there are other problems with the checks here. My understanding is a follows:
- A serving type is migrated iff:
a) The tablet_type / shard if found in the serving graph.
b) There are no tablet controls set for this tablet_type / shard (this might be optional, see my comment at the end)
One problem I see is that having found=true and noControls, should mean the shard is migrated. I think I'm forgetting something about how this works.
The second problem I see is the following condition:
if !tabletControl.GetQueryServiceDisabled() {
shardServedTypes = append(shardServedTypes, si.ShardName())
}
Isn't this adding to the list shards that actually have QueryService Enabled?
The presence of TabletControls should indicate that a migration is in progress, but ultimately the routing is determined by what is in the serving graph. The more I think about this, it seems that checking presence in the SrvKeyspaceGraph should be sufficient to determine if it was migrated 🤔
There was a problem hiding this comment.
@rohit-nayak-ps do you have a different understanding here? I'm more familiarized with the legacy MigrateServedTypes and maybe the logic changed.
…rkflow.Server` There is one small change in implementation here, where `noControls` would never be `false` in the old implementation. I believe this was a (harmless) bug, and the new implementation is more correct. Signed-off-by: Andrew Mason <amason@slack-corp.com>
…rkflow This required some updates of local variables from `workflow` to `workflowName` so that I could reference the `workflow` package name. I also reformatted imports in all the files I touched as part of this change. Signed-off-by: Andrew Mason <amason@slack-corp.com>
6860669 to
15e9ce6
Compare
Signed-off-by: Andrew Mason <amason@slack-corp.com>
There was a problem hiding this comment.
@ajm188 this looks good to me. There is an open question, but is actually outside the scope of this PR. We can revisit that offline with @rohit-nayak-ps.
I think we can merge this as is.
Description
Finally picking up this workstream again. This does pretty much what it says in the title, with a couple important notes to add here:
getCellsWithShardReadsSwitched, there was never a case wherenoControlswould be set to false (because it's initialized totrue, and there's only one branch where we sayif len(parition.GetShardTabletControls()) == 0) { noControls = true }. I believe this to be a (apparently harmless) bug, so I've changed the implementation to initializenoControlsto false. No tests broke when I made this change.getCellsWith{Shard,Table}ReadsSwitchedfunctions to take atopodatapb.TabletTypeargument rather than a string representing the tablet type. This made a call tostrings.EqualFoldfromget....ShardReadsSwitchedunnecessary, but made a call tostrings.ToLower(ttype.String())required. Just pushing the case-insensitive comparisons around, I guess.TrafficSwitchDirection/TableRemovalType), I needed to change a bunch of local variables fromworkflowtoworkflowName, so that the identifierworkflowcould refer to the package. This made the diff much larger as a result. I also took the opportunity to reformat our imports to be a little cleaner since I was already here.Related Issue(s)
Checklist
Deployment Notes