Added ValidateVSchema #8012
Added ValidateVSchema #8012
Conversation
makinje16
requested review from
ajm188,
deepthi,
doeg and
rohit-nayak-ps
as code owners
go/vt/vtctl/vtctl.go
Outdated
| @@ -2802,7 +2802,7 @@ func commandValidateSchemaKeyspace(ctx context.Context, wr *wrangler.Wrangler, s | |||
| if *excludeTables != "" { | |||
| excludeTableArray = strings.Split(*excludeTables, ",") | |||
| } | |||
| return wr.ValidateSchemaKeyspace(ctx, keyspace, excludeTableArray, *includeViews, *skipNoMaster) | |||
| return wr.ValidateSchemaKeyspace(ctx, keyspace, excludeTableArray, *includeViews, *skipNoMaster, false /*includeVSchema*/) | |||
There was a problem hiding this comment.
We should add a flag to this command (and probably ValidateSchemaShard, sorry for not pointing that out earlier) to be able to pass true through here.
makinje16
force-pushed
the
validate-keyspace-vschema-checks
branch
from
499c991
to
5d90c86
Compare
makinje16
changed the title
go/vt/vtctl/vtctl.go
Outdated
| } | ||
|
|
||
| func commandValidateSchemaKeyspace(ctx context.Context, wr *wrangler.Wrangler, subFlags *flag.FlagSet, args []string) error { | ||
| excludeTables := subFlags.String("exclude_tables", "", "Specifies a comma-separated list of tables to exclude. Each is either an exact match, or a regular expression of the form /regexp/") | ||
| includeViews := subFlags.Bool("include-views", false, "Includes views in the validation") | ||
| skipNoMaster := subFlags.Bool("skip-no-master", false, "Skip shards that don't have master when performing validation") | ||
| includeVSchema := subFlags.Bool("include-vschema", true, "Validate schemas against the vschema") |
go/vt/wrangler/schema.go
Outdated
| wg.Add(len(shards)) | ||
|
|
||
| for _, shard := range shards { | ||
| go func(shard string, shardFailures *concurrency.AllErrorRecorder) { |
There was a problem hiding this comment.
You shouldn't need to pass a reference to the recorder here; it's in scope in the closure of the goroutine (similar to how you're using the wait group).
There was a problem hiding this comment.
I actually got linting errors when I did not pass in the recorder or shard which is why I passed them in
Linting go/vt/wrangler
go/vt/wrangler/schema.go:289:45: loopclosure: loop variable shard captured by func literal (govet)
si, err := wr.ts.GetShard(ctx, keyspace, shard)
^
go/vt/wrangler/schema.go:291:83: loopclosure: loop variable shard captured by func literal (govet)
shardFailures.RecordError(fmt.Errorf("GetShard(%v, %v) failed: %v", keyspace, shard, err))
^
go/vt/wrangler/schema.go:297:45: loopclosure: loop variable shard captured by func literal (govet)
excludeTables, includeViews, keyspace, shard, err,
^
There was a problem hiding this comment.
Those lint errors are complaining about the shard variable specifically (which is a correct error). I'm talking about just the shardFailures variable which isn't part of the range loop.
To illustrate what the linter is warning about, check out the difference between these two loops: https://play.golang.org/p/eQtSzSI60lR
go/vt/wrangler/schema.go
Outdated
| } | ||
|
|
||
| // ValidateVSchemaKeyspace compares the schema of each primary tablet in "keyspace" and errs if there are differences | ||
| func (wr *Wrangler) ValidateVSchemaKeyspace(ctx context.Context, keyspace string, excludeTables []string, includeViews bool) error { |
There was a problem hiding this comment.
This is unused currently but thought that this functionality may be needed at some point. Instead of requiring the caller to pass in all shard names to ValidateVSchema we can use this. In the two instances in which I've seen us validate the vschema we also have had the shards we are trying to validate so I understand if we should remove this.
There was a problem hiding this comment.
Yeah I think if we don't need it now (and we don't expect to immediately need it in the short-term), then we should hold off. We can always add it back later!
go/vt/wrangler/schema.go
Outdated
| @@ -142,6 +142,12 @@ func (wr *Wrangler) diffSchema(ctx context.Context, masterSchema *tabletmanagerd | |||
| tmutils.DiffSchema(topoproto.TabletAliasString(masterTabletAlias), masterSchema, topoproto.TabletAliasString(alias), replicaSchema, er) | |||
| } | |||
|
|
|||
| // helper method to asynchronously validate a shards schema | |||
| func (wr *Wrangler) validateSchemaShardConcurrent(ctx context.Context, keyspace, shard string, excludeTables []string, includeViews bool, includeVSchema bool, wg *sync.WaitGroup) error { | |||
makinje16
force-pushed
the
validate-keyspace-vschema-checks
branch
2 times, most recently
from
59e4a05
to
3c847b2
Compare
makinje16
changed the title
Signed-off-by: Malcolm Akinje <makinje@slack-corp.com>
makinje16
force-pushed
the
validate-keyspace-vschema-checks
branch
from
3c847b2
to
3479947
Compare
askdba
added
Component: Cluster management
Type: Enhancement
Signed-off-by: Malcolm Akinje makinje@slack-corp.com
Description
This PR adds in
ValidateVSchema.ValidateSchemaShardnow uses this method as a helper whenincludeVSchemais provided to it. This also gives the oppourtunity to validate that a shards schema is equivalent to the vschema in other areas besides the aforementioned method like when we create a resharding workflow.Related Issue(s)
Checklist
Deployment Notes