VReplication SwitchTraffic: for a dry run check if sequences need to be updated #17842
+1,419
−1,169
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: Rohit Nayak <[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
NeedsBackportReason
rohit-nayak-ps
removed
NeedsWebsiteDocsUpdate
rohit-nayak-ps
removed
the
NeedsDescriptionUpdate
…value Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17842 +/- ##
==========================================
+ Coverage 67.45% 67.47% +0.01%
==========================================
Files 1592 1595 +3
Lines 258167 259066 +899
==========================================
+ Hits 174143 174792 +649
- Misses 84024 84274 +250 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rohit Nayak <[email protected]>
…e refactor Signed-off-by: Rohit Nayak <[email protected]>
mattlord
approved these changes
Feb 25, 2025
| if sm.usingTableDefinition != nil && sm.usingTableDefinition.AutoIncrement != nil { | ||
| usingCol, err = sqlescape.EnsureEscaped(sm.usingTableDefinition.AutoIncrement.Column) | ||
| if err != nil { | ||
| err = vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid column name %s specified for sequence in table %s: %v", |
There was a problem hiding this comment.
Nit, but INVALID_ARGUMENT is probably a better error code here and throughout this function.
Comment on lines
+92
to
+94
| err = vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid database name %s specified for sequence in table %s: %v", | ||
| sm.usingTableDBName, sm.usingTableName, err) | ||
| return err |
There was a problem hiding this comment.
Nit, but I don't see any reason not to return the errors directly in this function (return vterrors.Errorf...)
| errs := ts.ForAllTargets(func(target *MigrationTarget) error { | ||
| primary := target.GetPrimary() | ||
| if primary == nil || primary.GetAlias() == nil { | ||
| return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "no primary tablet found for target shard %s/%s", |
There was a problem hiding this comment.
Another nit but a better code here IMO would be FAILED_PRECONDITION.
Comment on lines
+267
to
+272
| sequenceShard, ierr := ts.TopoServer().GetOnlyShard(ctx, seq.backingTableKeyspace) | ||
| if ierr != nil || sequenceShard == nil || sequenceShard.PrimaryAlias == nil { | ||
| return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to get the primary tablet for keyspace %s: %v", | ||
| seq.backingTableKeyspace, ierr) | ||
| } | ||
| sequenceTablet, ierr := ts.TopoServer().GetTablet(ctx, sequenceShard.PrimaryAlias) |
There was a problem hiding this comment.
Not necessary to do here, but noting that we could save the tablets used in getCurrentSequenceValue and re-use them here so that we don't double the topo calls. If the total time taken is a concern, this refactor will make that problem worse (though topo calls should be fast vs mysqld calls).
|
|
||
| dr.drLog.Log("The following sequence backing tables used by tables being moved will be initialized:") | ||
| for _, backingTable := range sortedBackingTableNames { | ||
| dr.drLog.Logf("\t\tBacking table: %s, current value %d, new value %d", |
There was a problem hiding this comment.
Nit, but two tabs is a bit excessive IMO. I think 1 would be enough.
deepthi
approved these changes
Feb 26, 2025
|
|
||
| // createMissingSequenceTables will create the backing sequence tables for those that | ||
| // could not be found in any current keyspace. | ||
| func (ts trafficSwitcher) createMissingSequenceTables(ctx context.Context, sequencesByBackingTable map[string]*sequenceMetadata, tablesFound map[string]struct{}) error { |
There was a problem hiding this comment.
Suggested change
| func (ts trafficSwitcher) createMissingSequenceTables(ctx context.Context, sequencesByBackingTable map[string]*sequenceMetadata, tablesFound map[string]struct{}) error { | |
| func (ts *trafficSwitcher) createMissingSequenceTables(ctx context.Context, sequencesByBackingTable map[string]*sequenceMetadata, tablesFound map[string]struct{}) error { |
There was a problem hiding this comment.
GoLand produced a warning for this.
| return nil | ||
| } | ||
|
|
||
| func (ts trafficSwitcher) createSequenceTable(ctx context.Context, tableName string, primary *topo.TabletInfo) error { |
There was a problem hiding this comment.
Suggested change
| func (ts trafficSwitcher) createSequenceTable(ctx context.Context, tableName string, primary *topo.TabletInfo) error { | |
| func (ts *trafficSwitcher) createSequenceTable(ctx context.Context, tableName string, primary *topo.TabletInfo) error { |
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
When resharding an unsharded keyspace or moving a table from an unsharded keyspace to a sharded one, sequences need to replace autoincrements. These sequences need to be set to an initial value which is above the max id. Doing this for each table is tedious and the
--initial-target-sequencesoption was added toSwitchTrafficfor this purpose.However ff there are a lot of tables being resharded the process of getting the max ids from all shards and then updating the value on the unsharded keyspace can take too long. So it might still make sense for the users to update the sequence manually.
This PR adds the ability to find the sequences which need to be updated as part of switching traffic. The user can then decide on setting a value on the targets to a safe value and then not use the
--initial-target-sequencesflag.The code to get the max values was intertwined with the rest of the sequence setting code. So this PR also refactors the existing code to have a single code path for the dry run and regular path.
Related Issue(s)
fixes #17831
Checklist
Deployment Notes