VReplication: Improve MoveTables Create Error Handling #13737
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import ( | |
|
|
||
| "golang.org/x/sync/semaphore" | ||
| "google.golang.org/protobuf/encoding/prototext" | ||
| "google.golang.org/protobuf/proto" | ||
|
|
||
| "vitess.io/vitess/go/mysql/sqlerror" | ||
|
|
||
|
|
@@ -947,7 +948,7 @@ func (s *Server) getWorkflowCopyStates(ctx context.Context, tablet *topo.TabletI | |
| // MoveTablesCreate is part of the vtctlservicepb.VtctldServer interface. | ||
| // It passes the embedded TabletRequest object to the given keyspace's | ||
| // target primary tablets that will be executing the workflow. | ||
| func (s *Server) MoveTablesCreate(ctx context.Context, req *vtctldatapb.MoveTablesCreateRequest) (res *vtctldatapb.WorkflowStatusResponse, err error) { | ||
| span, ctx := trace.NewSpan(ctx, "workflow.Server.MoveTablesCreate") | ||
| defer span.Finish() | ||
|
|
||
|
|
@@ -964,7 +965,6 @@ func (s *Server) MoveTablesCreate(ctx context.Context, req *vtctldatapb.MoveTabl | |
| tables = req.IncludeTables | ||
| externalTopo *topo.Server | ||
| sourceTopo *topo.Server = s.ts | ||
| ) | ||
|
|
||
| // When the source is an external cluster mounted using the Mount command. | ||
|
|
@@ -978,6 +978,7 @@ func (s *Server) MoveTablesCreate(ctx context.Context, req *vtctldatapb.MoveTabl | |
| } | ||
|
|
||
| var vschema *vschemapb.Keyspace | ||
| var origVSchema *vschemapb.Keyspace // If we need to rollback a failed create | ||
| vschema, err = s.ts.GetVSchema(ctx, targetKeyspace) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -1020,43 +1021,14 @@ func (s *Server) MoveTablesCreate(ctx context.Context, req *vtctldatapb.MoveTabl | |
| log.Infof("Found tables to move: %s", strings.Join(tables, ",")) | ||
|
|
||
| if !vschema.Sharded { | ||
| // Save the original in case we need to restore it for a late failure | ||
| // in the defer(). | ||
| origVSchema = proto.Clone(vschema).(*vschemapb.Keyspace) | ||
| if err := s.addTablesToVSchema(ctx, sourceKeyspace, vschema, tables, externalTopo == nil); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| ms := &vtctldatapb.MaterializeSettings{ | ||
| Workflow: req.Workflow, | ||
| MaterializationIntent: vtctldatapb.MaterializationIntent_MOVETABLES, | ||
|
|
@@ -1101,6 +1073,68 @@ func (s *Server) MoveTablesCreate(ctx context.Context, req *vtctldatapb.MoveTabl | |
| return nil, err | ||
| } | ||
|
|
||
| // If we get an error after this point, where the vreplication streams/records | ||
| // have been created, then we clean up the workflow's artifacts. | ||
| defer func() { | ||
| if err != nil { | ||
| ts, cerr := s.buildTrafficSwitcher(ctx, ms.TargetKeyspace, ms.Workflow) | ||
| if cerr != nil { | ||
| err = vterrors.Wrapf(err, "failed to cleanup workflow artifacts: %v", cerr) | ||
| } | ||
| if cerr := s.dropArtifacts(ctx, false, &switcher{s: s, ts: ts}); cerr != nil { | ||
| err = vterrors.Wrapf(err, "failed to cleanup workflow artifacts: %v", cerr) | ||
| } | ||
| if origVSchema == nil { // There's no previous version to restore | ||
| return | ||
| } | ||
| if cerr := s.ts.SaveVSchema(ctx, targetKeyspace, origVSchema); cerr != nil { | ||
| err = vterrors.Wrapf(err, "failed to restore original target vschema: %v", cerr) | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| // Now that the streams have been successfully created, let's put the associated | ||
| // routing rules in place. | ||
| if externalTopo == nil { | ||
| // Save routing rules before vschema. If we save vschema first, and routing | ||
| // rules fails to save, we may generate duplicate table errors. | ||
|
There was a problem hiding this comment. There are no cases where if routing rules succeed to be saved, but then the vschema fails that things are inconsistent? There was a problem hiding this comment. That's a good question! It led me to realize that we can instead utilize the There was a problem hiding this comment. Turns out that wasn't quite right, but I did add code to deal with the vschema in the defer: 80f8a03 |
||
| if mz.isPartial { | ||
| if err := createDefaultShardRoutingRules(mz.ctx, mz.ms, mz.ts); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| rules, err := topotools.GetRoutingRules(ctx, s.ts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, table := range tables { | ||
| toSource := []string{sourceKeyspace + "." + table} | ||
| rules[table] = toSource | ||
| rules[table+"@replica"] = toSource | ||
| rules[table+"@rdonly"] = toSource | ||
| rules[targetKeyspace+"."+table] = toSource | ||
| rules[targetKeyspace+"."+table+"@replica"] = toSource | ||
| rules[targetKeyspace+"."+table+"@rdonly"] = toSource | ||
| rules[targetKeyspace+"."+table] = toSource | ||
| rules[sourceKeyspace+"."+table+"@replica"] = toSource | ||
| rules[sourceKeyspace+"."+table+"@rdonly"] = toSource | ||
| } | ||
| if err := topotools.SaveRoutingRules(ctx, s.ts, rules); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if vschema != nil { | ||
| // We added to the vschema. | ||
| if err := s.ts.SaveVSchema(ctx, targetKeyspace, vschema); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| } | ||
| if err := s.ts.RebuildSrvVSchema(ctx, nil); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if ms.SourceTimeZone != "" { | ||
| if err := mz.checkTZConversion(ctx, ms.SourceTimeZone); err != nil { | ||
| return nil, err | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressing an unrelated cause of test flakiness that was seen.