From e9f3c75253a0114f835fd156c94485dd2eee6789 Mon Sep 17 00:00:00 2001 From: Max Englander Date: Tue, 13 Jun 2023 11:25:16 -0400 Subject: [PATCH 1/7] go/vt/vtgate: export vindexes unknown params stat Signed-off-by: Max Englander --- go/vt/vtgate/executor.go | 7 +++++ go/vt/vtgate/vschema_stats.go | 18 ++++++++--- go/vt/vtgate/vtgate.go | 2 ++ go/vt/vtgate/vtgate_test.go | 58 +++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 5 deletions(-) diff --git a/go/vt/vtgate/executor.go b/go/vt/vtgate/executor.go index 644b1977ed9..32fb9bd33c2 100644 --- a/go/vt/vtgate/executor.go +++ b/go/vt/vtgate/executor.go @@ -947,6 +947,13 @@ func (e *Executor) SaveVSchema(vschema *vindexes.VSchema, stats *VSchemaStats) { vschemaCounters.Add("Reload", 1) } + if vschemaVindexUnknownParams != nil { + var unknownParams int + for _, ks := range stats.Keyspaces { + unknownParams += ks.VindexUnknownParamsCount + } + vschemaVindexUnknownParams.Set(int64(unknownParams)) + } } // ParseDestinationTarget parses destination target string and sets default keyspace if possible. diff --git a/go/vt/vtgate/vschema_stats.go b/go/vt/vtgate/vschema_stats.go index ce234fdba9a..aa0c84a5608 100644 --- a/go/vt/vtgate/vschema_stats.go +++ b/go/vt/vtgate/vschema_stats.go @@ -31,11 +31,12 @@ type VSchemaStats struct { // VSchemaKeyspaceStats contains a rollup of the VSchema stats for a keyspace. // It is used to display a table with the information in the status page. type VSchemaKeyspaceStats struct { - Keyspace string - Sharded bool - TableCount int - VindexCount int - Error string + Keyspace string + Sharded bool + TableCount int + VindexCount int + VindexUnknownParamsCount int + Error string } // NewVSchemaStats returns a new VSchemaStats from a VSchema. @@ -54,6 +55,11 @@ func NewVSchemaStats(vschema *vindexes.VSchema, errorMessage string) *VSchemaSta for _, t := range k.Tables { s.VindexCount += len(t.ColumnVindexes) + len(t.Ordered) + len(t.Owned) } + for _, vdx := range k.Vindexes { + if pv, ok := vdx.(vindexes.ParamValidating); ok { + s.VindexUnknownParamsCount += len(pv.UnknownParams()) + } + } } if k.Error != nil { s.Error = k.Error.Error() @@ -95,6 +101,7 @@ const ( Sharded Table Count Vindex Count + Vindex Unknown Params Count Error {{range $i, $ks := .Keyspaces}} @@ -102,6 +109,7 @@ const ( {{if $ks.Sharded}}Yes{{else}}No{{end}} {{$ks.TableCount}} {{$ks.VindexCount}} + {{$ks.VindexUnknownParamsCount}} {{$ks.Error}} {{end}} diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go index 02b4dc2ef26..1c910b6c629 100644 --- a/go/vt/vtgate/vtgate.go +++ b/go/vt/vtgate/vtgate.go @@ -179,6 +179,8 @@ var ( // catch the initial load stats. vschemaCounters = stats.NewCountersWithSingleLabel("VtgateVSchemaCounts", "Vtgate vschema counts", "changes") + vschemaVindexUnknownParams = stats.NewGauge("VSchemaVindexUnknownParams", "Number of params unrecognized by VSchema Vindexes") + // Error counters should be global so they can be set from anywhere errorCounts = stats.NewCountersWithMultiLabels("VtgateApiErrorCounts", "Vtgate API error counts per error type", []string{"Operation", "Keyspace", "DbType", "Code"}) diff --git a/go/vt/vtgate/vtgate_test.go b/go/vt/vtgate/vtgate_test.go index 74fef53f8b3..446f816bf76 100644 --- a/go/vt/vtgate/vtgate_test.go +++ b/go/vt/vtgate/vtgate_test.go @@ -644,6 +644,37 @@ var shardedVSchema = ` } ` +var shardedVSchemaUnknownParams = ` +{ + "sharded": true, + "vindexes": { + "hash_index": { + "type": "hash", + "params": { + "hello": "world", + "goodbye": "world" + } + }, + "binary_index": { + "type": "binary", + "params": { + "foo": "bar" + } + } + }, + "tables": { + "sp_tbl": { + "column_vindexes": [ + { + "column": "user_id", + "name": "hash_index" + } + ] + } + } +} +` + func TestMultiInternalSavepointVtGate(t *testing.T) { s := createSandbox(KsTestSharded) s.ShardSpec = "-40-80-" @@ -736,3 +767,30 @@ func TestMultiInternalSavepointVtGate(t *testing.T) { testQueryLog(t, logChan, "Execute", "INSERT", "insert into sp_tbl(user_id) values (:vtg1 /* INT64 */), (:vtg2 /* INT64 */)", 2) testQueryLog(t, logChan, "Execute", "INSERT", "insert into sp_tbl(user_id) values (:vtg1 /* INT64 */)", 1) } + +func TestVSchemaVindexUnknownParams(t *testing.T) { + s := createSandbox(KsTestSharded) + s.ShardSpec = "-40-80-" + + s.VSchema = shardedVSchema + srvSchema := getSandboxSrvVSchema() + rpcVTGate.executor.vm.VSchemaUpdate(srvSchema, nil) + hcVTGateTest.Reset() + + unknownParams := vschemaVindexUnknownParams.Get() + require.Equal(t, int64(0), unknownParams) + + s.VSchema = shardedVSchemaUnknownParams + srvSchema = getSandboxSrvVSchema() + rpcVTGate.executor.vm.VSchemaUpdate(srvSchema, nil) + + unknownParams = vschemaVindexUnknownParams.Get() + require.Equal(t, int64(3), unknownParams) + + s.VSchema = shardedVSchema + srvSchema = getSandboxSrvVSchema() + rpcVTGate.executor.vm.VSchemaUpdate(srvSchema, nil) + + unknownParams = vschemaVindexUnknownParams.Get() + require.Equal(t, int64(0), unknownParams) +} From f0a0c84b10a72cd65a0a7b64aeca37819a02fe8b Mon Sep 17 00:00:00 2001 From: Max Englander Date: Tue, 13 Jun 2023 22:47:09 -0400 Subject: [PATCH 2/7] go/vt/vtctl: ApplyVSchema surface unknown params Signed-off-by: Max Englander --- go/vt/topo/helpers/copy.go | 2 +- go/vt/topo/test/vschema.go | 4 +- go/vt/topo/topotests/srv_vschema_test.go | 6 +-- go/vt/topo/vschema.go | 21 +++++--- go/vt/vtcombo/tablet_map.go | 2 +- go/vt/vtctl/grpcvtctldserver/server.go | 4 +- go/vt/vtctl/grpcvtctldserver/server_test.go | 6 +-- go/vt/vtctl/vtctl.go | 18 ++++++- go/vt/vtctl/vtctl_test.go | 49 +++++++++++++++++++ go/vt/vtctld/api_test.go | 2 +- go/vt/vtgate/vindexes/vschema.go | 7 ++- go/vt/vtgate/vindexes/vschema_test.go | 6 +-- go/vt/vtgate/vschema_manager.go | 2 +- .../tabletserver/vstreamer/testenv/testenv.go | 2 +- go/vt/wrangler/materializer.go | 8 +-- go/vt/wrangler/materializer_test.go | 48 +++++++++--------- go/vt/wrangler/resharder_test.go | 16 +++--- go/vt/wrangler/traffic_switcher.go | 4 +- go/vt/wrangler/traffic_switcher_env_test.go | 6 +-- 19 files changed, 142 insertions(+), 71 deletions(-) diff --git a/go/vt/topo/helpers/copy.go b/go/vt/topo/helpers/copy.go index f0ae912243b..5270830a805 100644 --- a/go/vt/topo/helpers/copy.go +++ b/go/vt/topo/helpers/copy.go @@ -55,7 +55,7 @@ func CopyKeyspaces(ctx context.Context, fromTS, toTS *topo.Server) { vs, err := fromTS.GetVSchema(ctx, keyspace) switch { case err == nil: - if err := toTS.SaveVSchema(ctx, keyspace, vs); err != nil { + if err := toTS.ValidateAndSaveVSchema(ctx, keyspace, vs); err != nil { log.Errorf("SaveVSchema(%v): %v", keyspace, err) } case topo.IsErrType(err, topo.NoNode): diff --git a/go/vt/topo/test/vschema.go b/go/vt/topo/test/vschema.go index 0c2d58bdba7..3007a12dee3 100644 --- a/go/vt/topo/test/vschema.go +++ b/go/vt/topo/test/vschema.go @@ -46,7 +46,7 @@ func checkVSchema(t *testing.T, ts *topo.Server) { t.Error(err) } - err = ts.SaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ + err = ts.ValidateAndSaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ Tables: map[string]*vschemapb.Table{ "unsharded": {}, }, @@ -66,7 +66,7 @@ func checkVSchema(t *testing.T, ts *topo.Server) { t.Errorf("GetVSchema: %s, want %s", got, want) } - err = ts.SaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ + err = ts.ValidateAndSaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ Sharded: true, }) require.NoError(t, err) diff --git a/go/vt/topo/topotests/srv_vschema_test.go b/go/vt/topo/topotests/srv_vschema_test.go index 73745854ae1..7e81d3d0447 100644 --- a/go/vt/topo/topotests/srv_vschema_test.go +++ b/go/vt/topo/topotests/srv_vschema_test.go @@ -72,7 +72,7 @@ func TestRebuildVSchema(t *testing.T) { keyspace1 := &vschemapb.Keyspace{ Sharded: true, } - if err := ts.SaveVSchema(ctx, "ks1", keyspace1); err != nil { + if err := ts.ValidateAndSaveVSchema(ctx, "ks1", keyspace1); err != nil { t.Fatalf("SaveVSchema(ks1) failed: %v", err) } if err := ts.RebuildSrvVSchema(ctx, cells); err != nil { @@ -113,7 +113,7 @@ func TestRebuildVSchema(t *testing.T) { }, }, } - if err := ts.SaveVSchema(ctx, "ks2", keyspace2); err != nil { + if err := ts.ValidateAndSaveVSchema(ctx, "ks2", keyspace2); err != nil { t.Fatalf("SaveVSchema(ks1) failed: %v", err) } if err := ts.RebuildSrvVSchema(ctx, []string{"cell1"}); err != nil { @@ -175,7 +175,7 @@ func TestRebuildVSchema(t *testing.T) { wanted4.RoutingRules = rr // Delete a keyspace, checks vschema entry in map goes away. - if err := ts.SaveVSchema(ctx, "ks2", &vschemapb.Keyspace{}); err != nil { + if err := ts.ValidateAndSaveVSchema(ctx, "ks2", &vschemapb.Keyspace{}); err != nil { t.Fatalf("SaveVSchema(ks1) failed: %v", err) } if err := ts.DeleteKeyspace(ctx, "ks2"); err != nil { diff --git a/go/vt/topo/vschema.go b/go/vt/topo/vschema.go index a2503673deb..b6f8b6b7eb1 100644 --- a/go/vt/topo/vschema.go +++ b/go/vt/topo/vschema.go @@ -30,14 +30,10 @@ import ( "vitess.io/vitess/go/vt/vtgate/vindexes" ) -// SaveVSchema first validates the VSchema, then saves it. +// SaveVSchema saves the VSchema, without validating it. +// Should only be used if the VSchema was already validated. // If the VSchema is empty, just remove it. func (ts *Server) SaveVSchema(ctx context.Context, keyspace string, vschema *vschemapb.Keyspace) error { - err := vindexes.ValidateKeyspace(vschema) - if err != nil { - return err - } - nodePath := path.Join(KeyspacesPath, keyspace, VSchemaFile) data, err := vschema.MarshalVT() if err != nil { @@ -53,6 +49,17 @@ func (ts *Server) SaveVSchema(ctx context.Context, keyspace string, vschema *vsc return err } +// ValidateAndSaveVSchema first validates the VSchema, then saves it. +// If the VSchema is empty, just remove it. +func (ts *Server) ValidateAndSaveVSchema(ctx context.Context, keyspace string, vschema *vschemapb.Keyspace) error { + _, err := vindexes.BuildKeyspace(vschema) + if err != nil { + return err + } + + return ts.SaveVSchema(ctx, keyspace, vschema) +} + // DeleteVSchema delete the keyspace if it exists func (ts *Server) DeleteVSchema(ctx context.Context, keyspace string) error { log.Infof("deleting vschema for keyspace %s", keyspace) @@ -82,7 +89,7 @@ func (ts *Server) EnsureVSchema(ctx context.Context, keyspace string) error { log.Infof("error in getting vschema for keyspace %s: %v", keyspace, err) } if vschema == nil || IsErrType(err, NoNode) { - err = ts.SaveVSchema(ctx, keyspace, &vschemapb.Keyspace{ + err = ts.ValidateAndSaveVSchema(ctx, keyspace, &vschemapb.Keyspace{ Sharded: false, Vindexes: make(map[string]*vschemapb.Vindex), Tables: make(map[string]*vschemapb.Table), diff --git a/go/vt/vtcombo/tablet_map.go b/go/vt/vtcombo/tablet_map.go index 4c70a5230db..9d68ec8d5de 100644 --- a/go/vt/vtcombo/tablet_map.go +++ b/go/vt/vtcombo/tablet_map.go @@ -394,7 +394,7 @@ func CreateKs( return 0, fmt.Errorf("cannot load vschema file %v for keyspace %v: %v", f, keyspace, err) } - if err := ts.SaveVSchema(ctx, keyspace, formal); err != nil { + if err := ts.ValidateAndSaveVSchema(ctx, keyspace, formal); err != nil { return 0, fmt.Errorf("SaveVSchema(%v) failed: %v", keyspace, err) } } else { diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index 834abe5bb75..331ec7c3335 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -359,7 +359,7 @@ func (s *VtctldServer) ApplyVSchema(ctx context.Context, req *vtctldatapb.ApplyV return &vtctldatapb.ApplyVSchemaResponse{VSchema: vs}, nil } - if err = s.ts.SaveVSchema(ctx, req.Keyspace, vs); err != nil { + if err = s.ts.ValidateAndSaveVSchema(ctx, req.Keyspace, vs); err != nil { err = vterrors.Wrapf(err, "SaveVSchema(%s, %v)", req.Keyspace, req.VSchema) return nil, err } @@ -678,7 +678,7 @@ func (s *VtctldServer) CreateKeyspace(ctx context.Context, req *vtctldatapb.Crea // SNAPSHOT keyspaces are excluded from global routing. vs.RequireExplicitRouting = true - if err = s.ts.SaveVSchema(ctx, req.Name, vs); err != nil { + if err = s.ts.ValidateAndSaveVSchema(ctx, req.Name, vs); err != nil { err = fmt.Errorf("SaveVSchema(%v) = %w", vs, err) return nil, err } diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go index 48b53c53c05..41c90d9cb32 100644 --- a/go/vt/vtctl/grpcvtctldserver/server_test.go +++ b/go/vt/vtctl/grpcvtctldserver/server_test.go @@ -446,7 +446,7 @@ func TestApplyVSchema(t *testing.T) { }, }, } - err := ts.SaveVSchema(ctx, tt.req.Keyspace, origVSchema) + err := ts.ValidateAndSaveVSchema(ctx, tt.req.Keyspace, origVSchema) require.NoError(t, err) origSrvVSchema := &vschemapb.SrvVSchema{ @@ -1559,7 +1559,7 @@ func TestCreateKeyspace(t *testing.T) { } for name, vs := range tt.vschemas { - require.NoError(t, ts.SaveVSchema(ctx, name, vs), "error in SaveVSchema(%v, %+v)", name, vs) + require.NoError(t, ts.ValidateAndSaveVSchema(ctx, name, vs), "error in SaveVSchema(%v, %+v)", name, vs) } // Create the keyspace and make some assertions @@ -6060,7 +6060,7 @@ func TestGetVSchema(t *testing.T) { t.Run("found", func(t *testing.T) { t.Parallel() - err := ts.SaveVSchema(ctx, "testkeyspace", &vschemapb.Keyspace{ + err := ts.ValidateAndSaveVSchema(ctx, "testkeyspace", &vschemapb.Keyspace{ Sharded: true, Vindexes: map[string]*vschemapb.Vindex{ "v1": { diff --git a/go/vt/vtctl/vtctl.go b/go/vt/vtctl/vtctl.go index 14412902ef5..dd932e2d993 100644 --- a/go/vt/vtctl/vtctl.go +++ b/go/vt/vtctl/vtctl.go @@ -123,6 +123,7 @@ import ( "vitess.io/vitess/go/vt/topotools" "vitess.io/vitess/go/vt/vtctl/workflow" "vitess.io/vitess/go/vt/vterrors" + "vitess.io/vitess/go/vt/vtgate/vindexes" "vitess.io/vitess/go/vt/wrangler" ) @@ -1899,7 +1900,7 @@ func commandCreateKeyspace(ctx context.Context, wr *wrangler.Wrangler, subFlags // SNAPSHOT keyspaces are excluded from global routing. vs.RequireExplicitRouting = true } - if err := wr.TopoServer().SaveVSchema(ctx, keyspace, vs); err != nil { + if err := wr.TopoServer().ValidateAndSaveVSchema(ctx, keyspace, vs); err != nil { wr.Logger().Infof("error from SaveVSchema %v:%v", vs, err) return err } @@ -3335,6 +3336,21 @@ func commandApplyVSchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *p wr.Logger().Printf("New VSchema object:\n%s\nIf this is not what you expected, check the input data (as JSON parsing will skip unexpected fields).\n", b) } + // Validate the VSchema. + ksVs, err := vindexes.BuildKeyspace(vs) + if err != nil { + return err + } + + // Log unknown Vindex params as warnings. + for name, vdx := range ksVs.Vindexes { + if val, ok := vdx.(vindexes.ParamValidating); ok { + for _, param := range val.UnknownParams() { + wr.Logger().Warningf("Unknown param in vindex %s: %s", name, param) + } + } + } + if *dryRun { wr.Logger().Printf("Dry run: Skipping update of VSchema\n") return nil diff --git a/go/vt/vtctl/vtctl_test.go b/go/vt/vtctl/vtctl_test.go index ab2d7786a4b..f89a896e4a7 100644 --- a/go/vt/vtctl/vtctl_test.go +++ b/go/vt/vtctl/vtctl_test.go @@ -19,6 +19,7 @@ package vtctl import ( "context" "fmt" + "regexp" "strings" "testing" "time" @@ -32,6 +33,54 @@ import ( "vitess.io/vitess/go/vt/wrangler" ) +// TestApplyVSchema tests the the MoveTables client command +// via the commandVRApplyVSchema() cmd handler. +func TestApplyVSchema(t *testing.T) { + shard := "0" + ks := "ks" + ctx := context.Background() + env := newTestVTCtlEnv() + defer env.close() + _ = env.addTablet(100, ks, shard, &topodatapb.KeyRange{}, topodatapb.TabletType_PRIMARY) + + tests := []struct { + name string + args []string + expectResults func() + want string + }{ + { + name: "EmptyVSchema", + args: []string{"--vschema", "{}", ks}, + want: "New VSchema object:\n{}\nIf this is not what you expected, check the input data (as JSON parsing will skip unexpected fields).\n\n", + }, + { + name: "UnknownParamsLogged", + args: []string{"--vschema", "{\"sharded\":true,\"vindexes\":{\"hash_vdx\":{\"type\":\"hash\",\"params\":{\"foo\":\"bar\",\"hello\":\"world\"}},\"binary_vdx\":{\"type\":\"binary\",\"params\":{\"hello\":\"world\"}}}}", ks}, + want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: hello\nW0614 .* vtctl.go:.* Unknown param in vindex binary_vdx: hello", + }, + { + name: "UnknownParamsLoggedWithDryRun", + args: []string{"--vschema", "{\"sharded\":true,\"vindexes\":{\"hash_vdx\":{\"type\":\"hash\",\"params\":{\"foo\":\"bar\",\"hello\":\"world\"}},\"binary_vdx\":{\"type\":\"binary\",\"params\":{\"hello\":\"world\"}}}}", "--dry-run", ks}, + want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: hello\nW0614 .* vtctl.go:.* Unknown param in vindex binary_vdx: hello\nDry run: Skipping update of VSchema", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + subFlags := pflag.NewFlagSet("test", pflag.ContinueOnError) + err := commandApplyVSchema(ctx, env.wr, subFlags, tt.args) + require.NoError(t, err) + if strings.HasPrefix(tt.want, "/") { + require.Regexp(t, regexp.MustCompile(tt.want[1:]), env.cmdlog.String()) + } else { + require.Equal(t, tt.want, env.cmdlog.String()) + } + env.cmdlog.Clear() + env.tmc.clearResults() + }) + } +} + // TestMoveTables tests the the MoveTables client command // via the commandVRWorkflow() cmd handler. // This currently only tests the Progress action (which is diff --git a/go/vt/vtctld/api_test.go b/go/vt/vtctld/api_test.go index b00de6b0d09..0054a8814b1 100644 --- a/go/vt/vtctld/api_test.go +++ b/go/vt/vtctld/api_test.go @@ -78,7 +78,7 @@ func TestAPI(t *testing.T) { }, }, } - ts.SaveVSchema(ctx, "ks1", vs) + ts.ValidateAndSaveVSchema(ctx, "ks1", vs) tablet1 := topodatapb.Tablet{ Alias: &topodatapb.TabletAlias{Cell: "cell1", Uid: 100}, diff --git a/go/vt/vtgate/vindexes/vschema.go b/go/vt/vtgate/vindexes/vschema.go index dce0ec81690..fd8f515e1c7 100644 --- a/go/vt/vtgate/vindexes/vschema.go +++ b/go/vt/vtgate/vindexes/vschema.go @@ -284,11 +284,10 @@ func BuildKeyspaceSchema(input *vschemapb.Keyspace, keyspace string) (*KeyspaceS return vschema.Keyspaces[keyspace], err } -// ValidateKeyspace ensures that the keyspace vschema is valid. +// BuildKeyspace ensures that the keyspace vschema is valid. // External references (like sequence) are not validated. -func ValidateKeyspace(input *vschemapb.Keyspace) error { - _, err := BuildKeyspaceSchema(input, "") - return err +func BuildKeyspace(input *vschemapb.Keyspace) (*KeyspaceSchema, error) { + return BuildKeyspaceSchema(input, "") } func buildKeyspaces(source *vschemapb.SrvVSchema, vschema *VSchema) { diff --git a/go/vt/vtgate/vindexes/vschema_test.go b/go/vt/vtgate/vindexes/vschema_test.go index 335b42560cb..63d05767c82 100644 --- a/go/vt/vtgate/vindexes/vschema_test.go +++ b/go/vt/vtgate/vindexes/vschema_test.go @@ -234,7 +234,7 @@ func init() { } func TestUnshardedVSchemaValid(t *testing.T) { - err := ValidateKeyspace(&vschemapb.Keyspace{ + _, err := BuildKeyspace(&vschemapb.Keyspace{ Sharded: false, Vindexes: make(map[string]*vschemapb.Vindex), Tables: make(map[string]*vschemapb.Table), @@ -2432,7 +2432,7 @@ func TestValidate(t *testing.T) { "t2": {}, }, } - err := ValidateKeyspace(good) + _, err := BuildKeyspace(good) require.NoError(t, err) bad := &vschemapb.Keyspace{ Sharded: true, @@ -2445,7 +2445,7 @@ func TestValidate(t *testing.T) { "t2": {}, }, } - err = ValidateKeyspace(bad) + _, err = BuildKeyspace(bad) want := `vindexType "absent" not found` if err == nil || !strings.HasPrefix(err.Error(), want) { t.Errorf("Validate: %v, must start with %s", err, want) diff --git a/go/vt/vtgate/vschema_manager.go b/go/vt/vtgate/vschema_manager.go index baa232a87d8..c6cb20edb8d 100644 --- a/go/vt/vtgate/vschema_manager.go +++ b/go/vt/vtgate/vschema_manager.go @@ -70,7 +70,7 @@ func (vm *VSchemaManager) UpdateVSchema(ctx context.Context, ksName string, vsch } ks := vschema.Keyspaces[ksName] - err = topoServer.SaveVSchema(ctx, ksName, ks) + err = topoServer.ValidateAndSaveVSchema(ctx, ksName, ks) if err != nil { return err } diff --git a/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go b/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go index 9f2138ae19f..82f19900fbb 100644 --- a/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go +++ b/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go @@ -169,7 +169,7 @@ func (te *Env) SetVSchema(vs string) error { if err := json2.Unmarshal([]byte(vs), &kspb); err != nil { return err } - if err := te.TopoServ.SaveVSchema(ctx, te.KeyspaceName, &kspb); err != nil { + if err := te.TopoServ.ValidateAndSaveVSchema(ctx, te.KeyspaceName, &kspb); err != nil { return err } te.SchemaEngine.Reload(ctx) diff --git a/go/vt/wrangler/materializer.go b/go/vt/wrangler/materializer.go index 6dea5d3b9f1..a7246c495a4 100644 --- a/go/vt/wrangler/materializer.go +++ b/go/vt/wrangler/materializer.go @@ -237,7 +237,7 @@ func (wr *Wrangler) MoveTables(ctx context.Context, workflow, sourceKeyspace, ta if vschema != nil { // We added to the vschema. - if err := wr.ts.SaveVSchema(ctx, targetKeyspace, vschema); err != nil { + if err := wr.ts.ValidateAndSaveVSchema(ctx, targetKeyspace, vschema); err != nil { return err } } @@ -429,7 +429,7 @@ func (wr *Wrangler) CreateLookupVindex(ctx context.Context, keyspace string, spe if err != nil { return err } - if err := wr.ts.SaveVSchema(ctx, ms.TargetKeyspace, targetVSchema); err != nil { + if err := wr.ts.ValidateAndSaveVSchema(ctx, ms.TargetKeyspace, targetVSchema); err != nil { return err } ms.Cell = cell @@ -437,7 +437,7 @@ func (wr *Wrangler) CreateLookupVindex(ctx context.Context, keyspace string, spe if err := wr.Materialize(ctx, ms); err != nil { return err } - if err := wr.ts.SaveVSchema(ctx, keyspace, sourceVSchema); err != nil { + if err := wr.ts.ValidateAndSaveVSchema(ctx, keyspace, sourceVSchema); err != nil { return err } @@ -850,7 +850,7 @@ func (wr *Wrangler) ExternalizeVindex(ctx context.Context, qualifiedVindexName s // Remove the write_only param and save the source vschema. delete(sourceVindex.Params, "write_only") - if err := wr.ts.SaveVSchema(ctx, sourceKeyspace, sourceVSchema); err != nil { + if err := wr.ts.ValidateAndSaveVSchema(ctx, sourceKeyspace, sourceVSchema); err != nil { return err } return wr.ts.RebuildSrvVSchema(ctx, nil) diff --git a/go/vt/wrangler/materializer_test.go b/go/vt/wrangler/materializer_test.go index b2f90303802..54874acf39b 100644 --- a/go/vt/wrangler/materializer_test.go +++ b/go/vt/wrangler/materializer_test.go @@ -308,10 +308,10 @@ func TestCreateLookupVindexFull(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { t.Fatal(err) } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { t.Fatal(err) } @@ -391,7 +391,7 @@ func TestCreateLookupVindexCreateDDL(t *testing.T) { }, }, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vs); err != nil { t.Fatal(err) } @@ -806,10 +806,10 @@ func TestCreateLookupVindexSourceVSchema(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { t.Fatal(err) } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, tcase.sourceVSchema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, tcase.sourceVSchema); err != nil { t.Fatal(err) } @@ -844,7 +844,7 @@ func TestCreateLookupVindexTargetVSchema(t *testing.T) { }, }, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, sourcevs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, sourcevs); err != nil { t.Fatal(err) } @@ -1042,7 +1042,7 @@ func TestCreateLookupVindexTargetVSchema(t *testing.T) { }}, } specs.Vindexes["v"].Params["table"] = fmt.Sprintf("%s.%s", ms.TargetKeyspace, tcase.targetTable) - if err := env.topoServ.SaveVSchema(context.Background(), ms.TargetKeyspace, tcase.targetVSchema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.TargetKeyspace, tcase.targetVSchema); err != nil { t.Fatal(err) } @@ -1157,7 +1157,7 @@ func TestCreateLookupVindexSameKeyspace(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { t.Fatal(err) } @@ -1266,7 +1266,7 @@ func TestCreateCustomizedVindex(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { t.Fatal(err) } @@ -1340,7 +1340,7 @@ func TestStopAfterCopyFlag(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { t.Fatal(err) } @@ -1393,10 +1393,10 @@ func TestCreateLookupVindexFailures(t *testing.T) { }, }, } - if err := topoServ.SaveVSchema(context.Background(), "sourceks", vs); err != nil { + if err := topoServ.ValidateAndSaveVSchema(context.Background(), "sourceks", vs); err != nil { t.Fatal(err) } - if err := topoServ.SaveVSchema(context.Background(), "targetks", &vschemapb.Keyspace{}); err != nil { + if err := topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", &vschemapb.Keyspace{}); err != nil { t.Fatal(err) } @@ -1718,7 +1718,7 @@ func TestExternalizeVindex(t *testing.T) { }} for _, tcase := range testcases { // Resave the source schema for every iteration. - if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { t.Fatal(err) } if tcase.vrResponse != nil { @@ -1866,7 +1866,7 @@ func TestMaterializerOneToMany(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -1923,7 +1923,7 @@ func TestMaterializerManyToMany(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -1984,7 +1984,7 @@ func TestMaterializerMulticolumnVindex(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2121,7 +2121,7 @@ func TestMaterializerExplicitColumns(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2181,7 +2181,7 @@ func TestMaterializerRenamedColumns(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2253,7 +2253,7 @@ func TestMaterializerNoTargetVSchema(t *testing.T) { Sharded: true, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } env.tmc.expectVRQuery(200, mzSelectFrozenQuery, &sqltypes.Result{}) @@ -2471,7 +2471,7 @@ func TestMaterializerNoGoodVindex(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2512,7 +2512,7 @@ func TestMaterializerComplexVindexExpression(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2553,7 +2553,7 @@ func TestMaterializerNoVindexInExpression(t *testing.T) { }, } - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2774,7 +2774,7 @@ func TestMaterializerManyToManySomeUnreachable(t *testing.T) { for _, tcase := range testcases { t.Run("", func(t *testing.T) { env := newTestMaterializerEnv(t, ms, tcase.sourceShards, tcase.targetShards) - if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } defer env.close() @@ -3027,7 +3027,7 @@ func TestAddTablesToVSchema(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ts.SaveVSchema(ctx, srcks, tt.sourceVSchema) + ts.ValidateAndSaveVSchema(ctx, srcks, tt.sourceVSchema) err := wr.addTablesToVSchema(ctx, srcks, tt.inTargetVSchema, tt.tables, tt.copyVSchema) require.NoError(t, err) require.Equal(t, tt.wantTargetVSchema, tt.inTargetVSchema) diff --git a/go/vt/wrangler/resharder_test.go b/go/vt/wrangler/resharder_test.go index 9fdb5889a9e..085dece6728 100644 --- a/go/vt/wrangler/resharder_test.go +++ b/go/vt/wrangler/resharder_test.go @@ -211,7 +211,7 @@ func TestResharderOneRefTable(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -263,7 +263,7 @@ func TestReshardStopFlags(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -314,7 +314,7 @@ func TestResharderOneRefStream(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -391,7 +391,7 @@ func TestResharderNoRefStream(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -604,7 +604,7 @@ func TestResharderUnnamedStream(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -652,7 +652,7 @@ func TestResharderMismatchedRefStreams(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -768,7 +768,7 @@ func TestResharderMixedTablesOrder1(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -835,7 +835,7 @@ func TestResharderMixedTablesOrder2(t *testing.T) { }, }, } - if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } diff --git a/go/vt/wrangler/traffic_switcher.go b/go/vt/wrangler/traffic_switcher.go index c9bbb539807..d000388faf1 100644 --- a/go/vt/wrangler/traffic_switcher.go +++ b/go/vt/wrangler/traffic_switcher.go @@ -1712,7 +1712,7 @@ func (ts *trafficSwitcher) dropParticipatingTablesFromKeyspace(ctx context.Conte for _, tableName := range ts.Tables() { delete(vschema.Tables, tableName) } - return ts.TopoServer().SaveVSchema(ctx, keyspace, vschema) + return ts.TopoServer().ValidateAndSaveVSchema(ctx, keyspace, vschema) } // FIXME: even after dropSourceShards there are still entries in the topo, need to research and fix @@ -1866,5 +1866,5 @@ func (ts *trafficSwitcher) addParticipatingTablesToKeyspace(ctx context.Context, vschema.Tables[table] = &vschemapb.Table{} } } - return ts.TopoServer().SaveVSchema(ctx, keyspace, vschema) + return ts.TopoServer().ValidateAndSaveVSchema(ctx, keyspace, vschema) } diff --git a/go/vt/wrangler/traffic_switcher_env_test.go b/go/vt/wrangler/traffic_switcher_env_test.go index 7b219e47eec..2b4d565d4df 100644 --- a/go/vt/wrangler/traffic_switcher_env_test.go +++ b/go/vt/wrangler/traffic_switcher_env_test.go @@ -161,12 +161,12 @@ func newTestTableMigraterCustom(ctx context.Context, t *testing.T, sourceShards, }, } if len(sourceShards) != 1 { - if err := tme.ts.SaveVSchema(ctx, "ks1", vs); err != nil { + if err := tme.ts.ValidateAndSaveVSchema(ctx, "ks1", vs); err != nil { t.Fatal(err) } } if len(targetShards) != 1 { - if err := tme.ts.SaveVSchema(ctx, "ks2", vs); err != nil { + if err := tme.ts.ValidateAndSaveVSchema(ctx, "ks2", vs); err != nil { t.Fatal(err) } } @@ -329,7 +329,7 @@ func newTestShardMigrater(ctx context.Context, t *testing.T, sourceShards, targe }, }, } - if err := tme.ts.SaveVSchema(ctx, "ks", vs); err != nil { + if err := tme.ts.ValidateAndSaveVSchema(ctx, "ks", vs); err != nil { t.Fatal(err) } if err := tme.ts.RebuildSrvVSchema(ctx, nil); err != nil { From 715633cc60ad3271acec064805f7e1745ba5d297 Mon Sep 17 00:00:00 2001 From: Max Englander Date: Tue, 13 Jun 2023 23:17:47 -0400 Subject: [PATCH 3/7] pr feedback: consistent use of FindUnknownParams Signed-off-by: Max Englander --- go/vt/vtgate/vindexes/binarymd5.go | 10 +++++----- go/vt/vtgate/vindexes/cached_size.go | 22 ++++++---------------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/go/vt/vtgate/vindexes/binarymd5.go b/go/vt/vtgate/vindexes/binarymd5.go index 2dcaefb7a0d..d3495e28deb 100644 --- a/go/vt/vtgate/vindexes/binarymd5.go +++ b/go/vt/vtgate/vindexes/binarymd5.go @@ -33,15 +33,15 @@ var ( // BinaryMD5 is a vindex that hashes binary bits to a keyspace id. type BinaryMD5 struct { - name string - params map[string]string + name string + unknownParams []string } // newBinaryMD5 creates a new BinaryMD5. func newBinaryMD5(name string, params map[string]string) (Vindex, error) { return &BinaryMD5{ - name: name, - params: params, + name: name, + unknownParams: FindUnknownParams(params, nil), }, nil } @@ -101,7 +101,7 @@ func (vind *BinaryMD5) Hash(id sqltypes.Value) ([]byte, error) { // UnknownParams implements the ParamValidating interface. func (vind *BinaryMD5) UnknownParams() []string { - return FindUnknownParams(vind.params, nil) + return vind.unknownParams } func vMD5Hash(source []byte) []byte { diff --git a/go/vt/vtgate/vindexes/cached_size.go b/go/vt/vtgate/vindexes/cached_size.go index 492180e2d09..c27c5f33485 100644 --- a/go/vt/vtgate/vindexes/cached_size.go +++ b/go/vt/vtgate/vindexes/cached_size.go @@ -62,31 +62,21 @@ func (cached *Binary) CachedSize(alloc bool) int64 { } return size } - -//go:nocheckptr func (cached *BinaryMD5) CachedSize(alloc bool) int64 { if cached == nil { return int64(0) } size := int64(0) if alloc { - size += int64(24) + size += int64(48) } // field name string size += hack.RuntimeAllocSize(int64(len(cached.name))) - // field params map[string]string - if cached.params != nil { - size += int64(48) - hmap := reflect.ValueOf(cached.params) - numBuckets := int(math.Pow(2, float64((*(*uint8)(unsafe.Pointer(hmap.Pointer() + uintptr(9))))))) - numOldBuckets := (*(*uint16)(unsafe.Pointer(hmap.Pointer() + uintptr(10)))) - size += hack.RuntimeAllocSize(int64(numOldBuckets * 272)) - if len(cached.params) > 0 || numBuckets > 1 { - size += hack.RuntimeAllocSize(int64(numBuckets * 272)) - } - for k, v := range cached.params { - size += hack.RuntimeAllocSize(int64(len(k))) - size += hack.RuntimeAllocSize(int64(len(v))) + // field unknownParams []string + { + size += hack.RuntimeAllocSize(int64(cap(cached.unknownParams)) * int64(16)) + for _, elem := range cached.unknownParams { + size += hack.RuntimeAllocSize(int64(len(elem))) } } return size From 7b82fca04f39bc7dda40509d04475f79968fe382 Mon Sep 17 00:00:00 2001 From: Max Englander Date: Wed, 14 Jun 2023 00:55:51 -0400 Subject: [PATCH 4/7] changelog: document vtg VSchemaVindexUnknownParams stat Signed-off-by: Max Englander --- changelog/18.0/18.0.0/summary.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/changelog/18.0/18.0.0/summary.md b/changelog/18.0/18.0.0/summary.md index c33ef258cec..ab360a07484 100644 --- a/changelog/18.0/18.0.0/summary.md +++ b/changelog/18.0/18.0.0/summary.md @@ -8,7 +8,8 @@ - [VTOrc flag `--allow-emergency-reparent`](#new-flag-toggle-ers) - **[Deprecations and Deletions](#deprecations-and-deletions)** - [Deleted `k8stopo`](#deleted-k8stopo) - + - **[New stats](#new-stats)** + - [VTGate VSchema Vindex unknown params](#vtgate-vschema-vindex-unknown-params) ## Major Changes @@ -26,4 +27,10 @@ By default, VTOrc will be able to run `EmergencyReparentShard`. The users must s #### Deleted `k8stopo` -The `k8stopo` has been deprecated in Vitess 17, also see https://github.com/vitessio/vitess/issues/13298. With Vitess 18 the `k8stopo` has been removed. \ No newline at end of file +The `k8stopo` has been deprecated in Vitess 17, also see https://github.com/vitessio/vitess/issues/13298. With Vitess 18 the `k8stopo` has been removed. + +### New stats + +#### VTGate VSchema Vindex unknown params + +Gauges unknown Vindex params found in the latest VSchema pulled from the topology. \ No newline at end of file From 776e8c76bfd78fbc017f11fcc39b2e59e6031723 Mon Sep 17 00:00:00 2001 From: Max Englander Date: Tue, 20 Jun 2023 09:26:54 -0400 Subject: [PATCH 5/7] pr review: rename stat, readable tests, revert SaveVSchema Signed-off-by: Max Englander --- changelog/18.0/18.0.0/summary.md | 6 +-- go/vt/topo/helpers/copy.go | 2 +- go/vt/topo/test/vschema.go | 4 +- go/vt/topo/topotests/srv_vschema_test.go | 6 +-- go/vt/topo/vschema.go | 21 +++----- go/vt/vtcombo/tablet_map.go | 2 +- go/vt/vtctl/grpcvtctldserver/server.go | 4 +- go/vt/vtctl/grpcvtctldserver/server_test.go | 6 +-- ...unknown-params-logged-dry-run-vschema.json | 18 +++++++ .../unknown-params-logged-vschema.json | 18 +++++++ go/vt/vtctl/vtctl.go | 10 +++- go/vt/vtctl/vtctl_test.go | 17 +++++-- go/vt/vtctld/api_test.go | 2 +- go/vt/vtgate/executor.go | 4 +- go/vt/vtgate/vschema_manager.go | 2 +- go/vt/vtgate/vschema_stats.go | 2 +- go/vt/vtgate/vtgate.go | 4 +- go/vt/vtgate/vtgate_test.go | 6 +-- .../tabletserver/vstreamer/testenv/testenv.go | 2 +- go/vt/wrangler/materializer.go | 8 ++-- go/vt/wrangler/materializer_test.go | 48 +++++++++---------- go/vt/wrangler/resharder_test.go | 16 +++---- go/vt/wrangler/traffic_switcher.go | 4 +- go/vt/wrangler/traffic_switcher_env_test.go | 6 +-- 24 files changed, 131 insertions(+), 87 deletions(-) create mode 100644 go/vt/vtctl/testdata/unknown-params-logged-dry-run-vschema.json create mode 100644 go/vt/vtctl/testdata/unknown-params-logged-vschema.json diff --git a/changelog/18.0/18.0.0/summary.md b/changelog/18.0/18.0.0/summary.md index 5177c842258..b3b919afda9 100644 --- a/changelog/18.0/18.0.0/summary.md +++ b/changelog/18.0/18.0.0/summary.md @@ -10,7 +10,7 @@ - [Deleted `k8stopo`](#deleted-k8stopo) - [Deleted `vtgr`](#deleted-vtgr) - **[New stats](#new-stats)** - - [VTGate VSchema Vindex unknown params](#vtgate-vschema-vindex-unknown-params) + - [VTGate Vindex unknown params](#vtgate-vindex-unknown-params) ## Major Changes @@ -36,6 +36,6 @@ The `vtgr` has been deprecated in Vitess 17, also see https://github.com/vitessi ### New stats -#### VTGate VSchema Vindex unknown params +#### VTGate Vindex unknown params -Gauges unknown Vindex params found in the latest VSchema pulled from the topology. \ No newline at end of file +The VTGate stat `VindexUnknownParams` gauges unknown Vindex params found in the latest VSchema pulled from the topology. diff --git a/go/vt/topo/helpers/copy.go b/go/vt/topo/helpers/copy.go index 5270830a805..f0ae912243b 100644 --- a/go/vt/topo/helpers/copy.go +++ b/go/vt/topo/helpers/copy.go @@ -55,7 +55,7 @@ func CopyKeyspaces(ctx context.Context, fromTS, toTS *topo.Server) { vs, err := fromTS.GetVSchema(ctx, keyspace) switch { case err == nil: - if err := toTS.ValidateAndSaveVSchema(ctx, keyspace, vs); err != nil { + if err := toTS.SaveVSchema(ctx, keyspace, vs); err != nil { log.Errorf("SaveVSchema(%v): %v", keyspace, err) } case topo.IsErrType(err, topo.NoNode): diff --git a/go/vt/topo/test/vschema.go b/go/vt/topo/test/vschema.go index 3007a12dee3..0c2d58bdba7 100644 --- a/go/vt/topo/test/vschema.go +++ b/go/vt/topo/test/vschema.go @@ -46,7 +46,7 @@ func checkVSchema(t *testing.T, ts *topo.Server) { t.Error(err) } - err = ts.ValidateAndSaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ + err = ts.SaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ Tables: map[string]*vschemapb.Table{ "unsharded": {}, }, @@ -66,7 +66,7 @@ func checkVSchema(t *testing.T, ts *topo.Server) { t.Errorf("GetVSchema: %s, want %s", got, want) } - err = ts.ValidateAndSaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ + err = ts.SaveVSchema(ctx, "test_keyspace", &vschemapb.Keyspace{ Sharded: true, }) require.NoError(t, err) diff --git a/go/vt/topo/topotests/srv_vschema_test.go b/go/vt/topo/topotests/srv_vschema_test.go index 7e81d3d0447..73745854ae1 100644 --- a/go/vt/topo/topotests/srv_vschema_test.go +++ b/go/vt/topo/topotests/srv_vschema_test.go @@ -72,7 +72,7 @@ func TestRebuildVSchema(t *testing.T) { keyspace1 := &vschemapb.Keyspace{ Sharded: true, } - if err := ts.ValidateAndSaveVSchema(ctx, "ks1", keyspace1); err != nil { + if err := ts.SaveVSchema(ctx, "ks1", keyspace1); err != nil { t.Fatalf("SaveVSchema(ks1) failed: %v", err) } if err := ts.RebuildSrvVSchema(ctx, cells); err != nil { @@ -113,7 +113,7 @@ func TestRebuildVSchema(t *testing.T) { }, }, } - if err := ts.ValidateAndSaveVSchema(ctx, "ks2", keyspace2); err != nil { + if err := ts.SaveVSchema(ctx, "ks2", keyspace2); err != nil { t.Fatalf("SaveVSchema(ks1) failed: %v", err) } if err := ts.RebuildSrvVSchema(ctx, []string{"cell1"}); err != nil { @@ -175,7 +175,7 @@ func TestRebuildVSchema(t *testing.T) { wanted4.RoutingRules = rr // Delete a keyspace, checks vschema entry in map goes away. - if err := ts.ValidateAndSaveVSchema(ctx, "ks2", &vschemapb.Keyspace{}); err != nil { + if err := ts.SaveVSchema(ctx, "ks2", &vschemapb.Keyspace{}); err != nil { t.Fatalf("SaveVSchema(ks1) failed: %v", err) } if err := ts.DeleteKeyspace(ctx, "ks2"); err != nil { diff --git a/go/vt/topo/vschema.go b/go/vt/topo/vschema.go index b6f8b6b7eb1..c167475a462 100644 --- a/go/vt/topo/vschema.go +++ b/go/vt/topo/vschema.go @@ -30,10 +30,14 @@ import ( "vitess.io/vitess/go/vt/vtgate/vindexes" ) -// SaveVSchema saves the VSchema, without validating it. -// Should only be used if the VSchema was already validated. +// SaveVSchema first validates the VSchema, then saves it. // If the VSchema is empty, just remove it. func (ts *Server) SaveVSchema(ctx context.Context, keyspace string, vschema *vschemapb.Keyspace) error { + _, err := vindexes.BuildKeyspace(vschema) + if err != nil { + return err + } + nodePath := path.Join(KeyspacesPath, keyspace, VSchemaFile) data, err := vschema.MarshalVT() if err != nil { @@ -49,17 +53,6 @@ func (ts *Server) SaveVSchema(ctx context.Context, keyspace string, vschema *vsc return err } -// ValidateAndSaveVSchema first validates the VSchema, then saves it. -// If the VSchema is empty, just remove it. -func (ts *Server) ValidateAndSaveVSchema(ctx context.Context, keyspace string, vschema *vschemapb.Keyspace) error { - _, err := vindexes.BuildKeyspace(vschema) - if err != nil { - return err - } - - return ts.SaveVSchema(ctx, keyspace, vschema) -} - // DeleteVSchema delete the keyspace if it exists func (ts *Server) DeleteVSchema(ctx context.Context, keyspace string) error { log.Infof("deleting vschema for keyspace %s", keyspace) @@ -89,7 +82,7 @@ func (ts *Server) EnsureVSchema(ctx context.Context, keyspace string) error { log.Infof("error in getting vschema for keyspace %s: %v", keyspace, err) } if vschema == nil || IsErrType(err, NoNode) { - err = ts.ValidateAndSaveVSchema(ctx, keyspace, &vschemapb.Keyspace{ + err = ts.SaveVSchema(ctx, keyspace, &vschemapb.Keyspace{ Sharded: false, Vindexes: make(map[string]*vschemapb.Vindex), Tables: make(map[string]*vschemapb.Table), diff --git a/go/vt/vtcombo/tablet_map.go b/go/vt/vtcombo/tablet_map.go index 9d68ec8d5de..4c70a5230db 100644 --- a/go/vt/vtcombo/tablet_map.go +++ b/go/vt/vtcombo/tablet_map.go @@ -394,7 +394,7 @@ func CreateKs( return 0, fmt.Errorf("cannot load vschema file %v for keyspace %v: %v", f, keyspace, err) } - if err := ts.ValidateAndSaveVSchema(ctx, keyspace, formal); err != nil { + if err := ts.SaveVSchema(ctx, keyspace, formal); err != nil { return 0, fmt.Errorf("SaveVSchema(%v) failed: %v", keyspace, err) } } else { diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index 331ec7c3335..834abe5bb75 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -359,7 +359,7 @@ func (s *VtctldServer) ApplyVSchema(ctx context.Context, req *vtctldatapb.ApplyV return &vtctldatapb.ApplyVSchemaResponse{VSchema: vs}, nil } - if err = s.ts.ValidateAndSaveVSchema(ctx, req.Keyspace, vs); err != nil { + if err = s.ts.SaveVSchema(ctx, req.Keyspace, vs); err != nil { err = vterrors.Wrapf(err, "SaveVSchema(%s, %v)", req.Keyspace, req.VSchema) return nil, err } @@ -678,7 +678,7 @@ func (s *VtctldServer) CreateKeyspace(ctx context.Context, req *vtctldatapb.Crea // SNAPSHOT keyspaces are excluded from global routing. vs.RequireExplicitRouting = true - if err = s.ts.ValidateAndSaveVSchema(ctx, req.Name, vs); err != nil { + if err = s.ts.SaveVSchema(ctx, req.Name, vs); err != nil { err = fmt.Errorf("SaveVSchema(%v) = %w", vs, err) return nil, err } diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go index 41c90d9cb32..48b53c53c05 100644 --- a/go/vt/vtctl/grpcvtctldserver/server_test.go +++ b/go/vt/vtctl/grpcvtctldserver/server_test.go @@ -446,7 +446,7 @@ func TestApplyVSchema(t *testing.T) { }, }, } - err := ts.ValidateAndSaveVSchema(ctx, tt.req.Keyspace, origVSchema) + err := ts.SaveVSchema(ctx, tt.req.Keyspace, origVSchema) require.NoError(t, err) origSrvVSchema := &vschemapb.SrvVSchema{ @@ -1559,7 +1559,7 @@ func TestCreateKeyspace(t *testing.T) { } for name, vs := range tt.vschemas { - require.NoError(t, ts.ValidateAndSaveVSchema(ctx, name, vs), "error in SaveVSchema(%v, %+v)", name, vs) + require.NoError(t, ts.SaveVSchema(ctx, name, vs), "error in SaveVSchema(%v, %+v)", name, vs) } // Create the keyspace and make some assertions @@ -6060,7 +6060,7 @@ func TestGetVSchema(t *testing.T) { t.Run("found", func(t *testing.T) { t.Parallel() - err := ts.ValidateAndSaveVSchema(ctx, "testkeyspace", &vschemapb.Keyspace{ + err := ts.SaveVSchema(ctx, "testkeyspace", &vschemapb.Keyspace{ Sharded: true, Vindexes: map[string]*vschemapb.Vindex{ "v1": { diff --git a/go/vt/vtctl/testdata/unknown-params-logged-dry-run-vschema.json b/go/vt/vtctl/testdata/unknown-params-logged-dry-run-vschema.json new file mode 100644 index 00000000000..aefcfb13ae7 --- /dev/null +++ b/go/vt/vtctl/testdata/unknown-params-logged-dry-run-vschema.json @@ -0,0 +1,18 @@ +{ + "sharded": true, + "vindexes": { + "hash_vdx" : { + "type": "hash", + "params": { + "foo": "bar", + "hello": "world" + } + }, + "binary_vdx": { + "type": "binary", + "params": { + "hello": "world" + } + } + } +} diff --git a/go/vt/vtctl/testdata/unknown-params-logged-vschema.json b/go/vt/vtctl/testdata/unknown-params-logged-vschema.json new file mode 100644 index 00000000000..d3abc1c0e03 --- /dev/null +++ b/go/vt/vtctl/testdata/unknown-params-logged-vschema.json @@ -0,0 +1,18 @@ +{ + "sharded": true, + "vindexes": { + "binary_vdx": { + "type": "binary", + "params": { + "hello": "world" + } + }, + "hash_vdx": { + "type": "hash", + "params": { + "foo": "bar", + "hello": "world" + } + } + } +} diff --git a/go/vt/vtctl/vtctl.go b/go/vt/vtctl/vtctl.go index dd932e2d993..70ae6c90832 100644 --- a/go/vt/vtctl/vtctl.go +++ b/go/vt/vtctl/vtctl.go @@ -1900,7 +1900,7 @@ func commandCreateKeyspace(ctx context.Context, wr *wrangler.Wrangler, subFlags // SNAPSHOT keyspaces are excluded from global routing. vs.RequireExplicitRouting = true } - if err := wr.TopoServer().ValidateAndSaveVSchema(ctx, keyspace, vs); err != nil { + if err := wr.TopoServer().SaveVSchema(ctx, keyspace, vs); err != nil { wr.Logger().Infof("error from SaveVSchema %v:%v", vs, err) return err } @@ -3343,7 +3343,13 @@ func commandApplyVSchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *p } // Log unknown Vindex params as warnings. - for name, vdx := range ksVs.Vindexes { + var vdxNames []string + for name := range ksVs.Vindexes { + vdxNames = append(vdxNames, name) + } + sort.Strings(vdxNames) + for _, name := range vdxNames { + vdx := ksVs.Vindexes[name] if val, ok := vdx.(vindexes.ParamValidating); ok { for _, param := range val.UnknownParams() { wr.Logger().Warningf("Unknown param in vindex %s: %s", name, param) diff --git a/go/vt/vtctl/vtctl_test.go b/go/vt/vtctl/vtctl_test.go index f89a896e4a7..309363540b1 100644 --- a/go/vt/vtctl/vtctl_test.go +++ b/go/vt/vtctl/vtctl_test.go @@ -18,6 +18,7 @@ package vtctl import ( "context" + _ "embed" "fmt" "regexp" "strings" @@ -33,6 +34,14 @@ import ( "vitess.io/vitess/go/vt/wrangler" ) +var ( + //go:embed testdata/unknown-params-logged-vschema.json + unknownParamsLoggedVSchema string + + //go:embed testdata/unknown-params-logged-dry-run-vschema.json + unknownParamsLoggedDryRunVSchema string +) + // TestApplyVSchema tests the the MoveTables client command // via the commandVRApplyVSchema() cmd handler. func TestApplyVSchema(t *testing.T) { @@ -56,13 +65,13 @@ func TestApplyVSchema(t *testing.T) { }, { name: "UnknownParamsLogged", - args: []string{"--vschema", "{\"sharded\":true,\"vindexes\":{\"hash_vdx\":{\"type\":\"hash\",\"params\":{\"foo\":\"bar\",\"hello\":\"world\"}},\"binary_vdx\":{\"type\":\"binary\",\"params\":{\"hello\":\"world\"}}}}", ks}, - want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: hello\nW0614 .* vtctl.go:.* Unknown param in vindex binary_vdx: hello", + args: []string{"--vschema", unknownParamsLoggedVSchema, ks}, + want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W.* .* vtctl.go:.* Unknown param in vindex binary_vdx: hello\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: hello", }, { name: "UnknownParamsLoggedWithDryRun", - args: []string{"--vschema", "{\"sharded\":true,\"vindexes\":{\"hash_vdx\":{\"type\":\"hash\",\"params\":{\"foo\":\"bar\",\"hello\":\"world\"}},\"binary_vdx\":{\"type\":\"binary\",\"params\":{\"hello\":\"world\"}}}}", "--dry-run", ks}, - want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW0614 .* vtctl.go:.* Unknown param in vindex hash_vdx: hello\nW0614 .* vtctl.go:.* Unknown param in vindex binary_vdx: hello\nDry run: Skipping update of VSchema", + args: []string{"--vschema", unknownParamsLoggedDryRunVSchema, "--dry-run", ks}, + want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W.* .* vtctl.go:.* Unknown param in vindex binary_vdx: hello\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: hello\nDry run: Skipping update of VSchema", }, } for _, tt := range tests { diff --git a/go/vt/vtctld/api_test.go b/go/vt/vtctld/api_test.go index 0054a8814b1..b00de6b0d09 100644 --- a/go/vt/vtctld/api_test.go +++ b/go/vt/vtctld/api_test.go @@ -78,7 +78,7 @@ func TestAPI(t *testing.T) { }, }, } - ts.ValidateAndSaveVSchema(ctx, "ks1", vs) + ts.SaveVSchema(ctx, "ks1", vs) tablet1 := topodatapb.Tablet{ Alias: &topodatapb.TabletAlias{Cell: "cell1", Uid: 100}, diff --git a/go/vt/vtgate/executor.go b/go/vt/vtgate/executor.go index 32fb9bd33c2..a59aa49c525 100644 --- a/go/vt/vtgate/executor.go +++ b/go/vt/vtgate/executor.go @@ -947,12 +947,12 @@ func (e *Executor) SaveVSchema(vschema *vindexes.VSchema, stats *VSchemaStats) { vschemaCounters.Add("Reload", 1) } - if vschemaVindexUnknownParams != nil { + if vindexUnknownParams != nil { var unknownParams int for _, ks := range stats.Keyspaces { unknownParams += ks.VindexUnknownParamsCount } - vschemaVindexUnknownParams.Set(int64(unknownParams)) + vindexUnknownParams.Set(int64(unknownParams)) } } diff --git a/go/vt/vtgate/vschema_manager.go b/go/vt/vtgate/vschema_manager.go index c6cb20edb8d..baa232a87d8 100644 --- a/go/vt/vtgate/vschema_manager.go +++ b/go/vt/vtgate/vschema_manager.go @@ -70,7 +70,7 @@ func (vm *VSchemaManager) UpdateVSchema(ctx context.Context, ksName string, vsch } ks := vschema.Keyspaces[ksName] - err = topoServer.ValidateAndSaveVSchema(ctx, ksName, ks) + err = topoServer.SaveVSchema(ctx, ksName, ks) if err != nil { return err } diff --git a/go/vt/vtgate/vschema_stats.go b/go/vt/vtgate/vschema_stats.go index aa0c84a5608..d4920d7486f 100644 --- a/go/vt/vtgate/vschema_stats.go +++ b/go/vt/vtgate/vschema_stats.go @@ -101,7 +101,7 @@ const ( Sharded Table Count Vindex Count - Vindex Unknown Params Count + Vindex Unknown Parameters Count Error {{range $i, $ks := .Keyspaces}} diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go index 1c910b6c629..f15bde6b057 100644 --- a/go/vt/vtgate/vtgate.go +++ b/go/vt/vtgate/vtgate.go @@ -179,8 +179,6 @@ var ( // catch the initial load stats. vschemaCounters = stats.NewCountersWithSingleLabel("VtgateVSchemaCounts", "Vtgate vschema counts", "changes") - vschemaVindexUnknownParams = stats.NewGauge("VSchemaVindexUnknownParams", "Number of params unrecognized by VSchema Vindexes") - // Error counters should be global so they can be set from anywhere errorCounts = stats.NewCountersWithMultiLabels("VtgateApiErrorCounts", "Vtgate API error counts per error type", []string{"Operation", "Keyspace", "DbType", "Code"}) @@ -188,6 +186,8 @@ var ( vstreamSkewDelayCount = stats.NewCounter("VStreamEventsDelayedBySkewAlignment", "Number of events that had to wait because the skew across shards was too high") + + vindexUnknownParams = stats.NewGauge("VindexUnknownParams", "Number of params unrecognized by Vindexes") ) // VTGate is the rpc interface to vtgate. Only one instance diff --git a/go/vt/vtgate/vtgate_test.go b/go/vt/vtgate/vtgate_test.go index 446f816bf76..94451706117 100644 --- a/go/vt/vtgate/vtgate_test.go +++ b/go/vt/vtgate/vtgate_test.go @@ -777,20 +777,20 @@ func TestVSchemaVindexUnknownParams(t *testing.T) { rpcVTGate.executor.vm.VSchemaUpdate(srvSchema, nil) hcVTGateTest.Reset() - unknownParams := vschemaVindexUnknownParams.Get() + unknownParams := vindexUnknownParams.Get() require.Equal(t, int64(0), unknownParams) s.VSchema = shardedVSchemaUnknownParams srvSchema = getSandboxSrvVSchema() rpcVTGate.executor.vm.VSchemaUpdate(srvSchema, nil) - unknownParams = vschemaVindexUnknownParams.Get() + unknownParams = vindexUnknownParams.Get() require.Equal(t, int64(3), unknownParams) s.VSchema = shardedVSchema srvSchema = getSandboxSrvVSchema() rpcVTGate.executor.vm.VSchemaUpdate(srvSchema, nil) - unknownParams = vschemaVindexUnknownParams.Get() + unknownParams = vindexUnknownParams.Get() require.Equal(t, int64(0), unknownParams) } diff --git a/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go b/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go index 82f19900fbb..9f2138ae19f 100644 --- a/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go +++ b/go/vt/vttablet/tabletserver/vstreamer/testenv/testenv.go @@ -169,7 +169,7 @@ func (te *Env) SetVSchema(vs string) error { if err := json2.Unmarshal([]byte(vs), &kspb); err != nil { return err } - if err := te.TopoServ.ValidateAndSaveVSchema(ctx, te.KeyspaceName, &kspb); err != nil { + if err := te.TopoServ.SaveVSchema(ctx, te.KeyspaceName, &kspb); err != nil { return err } te.SchemaEngine.Reload(ctx) diff --git a/go/vt/wrangler/materializer.go b/go/vt/wrangler/materializer.go index a7246c495a4..6dea5d3b9f1 100644 --- a/go/vt/wrangler/materializer.go +++ b/go/vt/wrangler/materializer.go @@ -237,7 +237,7 @@ func (wr *Wrangler) MoveTables(ctx context.Context, workflow, sourceKeyspace, ta if vschema != nil { // We added to the vschema. - if err := wr.ts.ValidateAndSaveVSchema(ctx, targetKeyspace, vschema); err != nil { + if err := wr.ts.SaveVSchema(ctx, targetKeyspace, vschema); err != nil { return err } } @@ -429,7 +429,7 @@ func (wr *Wrangler) CreateLookupVindex(ctx context.Context, keyspace string, spe if err != nil { return err } - if err := wr.ts.ValidateAndSaveVSchema(ctx, ms.TargetKeyspace, targetVSchema); err != nil { + if err := wr.ts.SaveVSchema(ctx, ms.TargetKeyspace, targetVSchema); err != nil { return err } ms.Cell = cell @@ -437,7 +437,7 @@ func (wr *Wrangler) CreateLookupVindex(ctx context.Context, keyspace string, spe if err := wr.Materialize(ctx, ms); err != nil { return err } - if err := wr.ts.ValidateAndSaveVSchema(ctx, keyspace, sourceVSchema); err != nil { + if err := wr.ts.SaveVSchema(ctx, keyspace, sourceVSchema); err != nil { return err } @@ -850,7 +850,7 @@ func (wr *Wrangler) ExternalizeVindex(ctx context.Context, qualifiedVindexName s // Remove the write_only param and save the source vschema. delete(sourceVindex.Params, "write_only") - if err := wr.ts.ValidateAndSaveVSchema(ctx, sourceKeyspace, sourceVSchema); err != nil { + if err := wr.ts.SaveVSchema(ctx, sourceKeyspace, sourceVSchema); err != nil { return err } return wr.ts.RebuildSrvVSchema(ctx, nil) diff --git a/go/vt/wrangler/materializer_test.go b/go/vt/wrangler/materializer_test.go index 54874acf39b..b2f90303802 100644 --- a/go/vt/wrangler/materializer_test.go +++ b/go/vt/wrangler/materializer_test.go @@ -308,10 +308,10 @@ func TestCreateLookupVindexFull(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { t.Fatal(err) } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { t.Fatal(err) } @@ -391,7 +391,7 @@ func TestCreateLookupVindexCreateDDL(t *testing.T) { }, }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vs); err != nil { t.Fatal(err) } @@ -806,10 +806,10 @@ func TestCreateLookupVindexSourceVSchema(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.TargetKeyspace, &vschemapb.Keyspace{}); err != nil { t.Fatal(err) } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, tcase.sourceVSchema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, tcase.sourceVSchema); err != nil { t.Fatal(err) } @@ -844,7 +844,7 @@ func TestCreateLookupVindexTargetVSchema(t *testing.T) { }, }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, sourcevs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, sourcevs); err != nil { t.Fatal(err) } @@ -1042,7 +1042,7 @@ func TestCreateLookupVindexTargetVSchema(t *testing.T) { }}, } specs.Vindexes["v"].Params["table"] = fmt.Sprintf("%s.%s", ms.TargetKeyspace, tcase.targetTable) - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.TargetKeyspace, tcase.targetVSchema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.TargetKeyspace, tcase.targetVSchema); err != nil { t.Fatal(err) } @@ -1157,7 +1157,7 @@ func TestCreateLookupVindexSameKeyspace(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { t.Fatal(err) } @@ -1266,7 +1266,7 @@ func TestCreateCustomizedVindex(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { t.Fatal(err) } @@ -1340,7 +1340,7 @@ func TestStopAfterCopyFlag(t *testing.T) { Schema: sourceSchema, }}, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, vschema); err != nil { t.Fatal(err) } @@ -1393,10 +1393,10 @@ func TestCreateLookupVindexFailures(t *testing.T) { }, }, } - if err := topoServ.ValidateAndSaveVSchema(context.Background(), "sourceks", vs); err != nil { + if err := topoServ.SaveVSchema(context.Background(), "sourceks", vs); err != nil { t.Fatal(err) } - if err := topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", &vschemapb.Keyspace{}); err != nil { + if err := topoServ.SaveVSchema(context.Background(), "targetks", &vschemapb.Keyspace{}); err != nil { t.Fatal(err) } @@ -1718,7 +1718,7 @@ func TestExternalizeVindex(t *testing.T) { }} for _, tcase := range testcases { // Resave the source schema for every iteration. - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), ms.SourceKeyspace, sourceVSchema); err != nil { t.Fatal(err) } if tcase.vrResponse != nil { @@ -1866,7 +1866,7 @@ func TestMaterializerOneToMany(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -1923,7 +1923,7 @@ func TestMaterializerManyToMany(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -1984,7 +1984,7 @@ func TestMaterializerMulticolumnVindex(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2121,7 +2121,7 @@ func TestMaterializerExplicitColumns(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2181,7 +2181,7 @@ func TestMaterializerRenamedColumns(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2253,7 +2253,7 @@ func TestMaterializerNoTargetVSchema(t *testing.T) { Sharded: true, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } env.tmc.expectVRQuery(200, mzSelectFrozenQuery, &sqltypes.Result{}) @@ -2471,7 +2471,7 @@ func TestMaterializerNoGoodVindex(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2512,7 +2512,7 @@ func TestMaterializerComplexVindexExpression(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2553,7 +2553,7 @@ func TestMaterializerNoVindexInExpression(t *testing.T) { }, } - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } @@ -2774,7 +2774,7 @@ func TestMaterializerManyToManySomeUnreachable(t *testing.T) { for _, tcase := range testcases { t.Run("", func(t *testing.T) { env := newTestMaterializerEnv(t, ms, tcase.sourceShards, tcase.targetShards) - if err := env.topoServ.ValidateAndSaveVSchema(context.Background(), "targetks", vs); err != nil { + if err := env.topoServ.SaveVSchema(context.Background(), "targetks", vs); err != nil { t.Fatal(err) } defer env.close() @@ -3027,7 +3027,7 @@ func TestAddTablesToVSchema(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ts.ValidateAndSaveVSchema(ctx, srcks, tt.sourceVSchema) + ts.SaveVSchema(ctx, srcks, tt.sourceVSchema) err := wr.addTablesToVSchema(ctx, srcks, tt.inTargetVSchema, tt.tables, tt.copyVSchema) require.NoError(t, err) require.Equal(t, tt.wantTargetVSchema, tt.inTargetVSchema) diff --git a/go/vt/wrangler/resharder_test.go b/go/vt/wrangler/resharder_test.go index 085dece6728..9fdb5889a9e 100644 --- a/go/vt/wrangler/resharder_test.go +++ b/go/vt/wrangler/resharder_test.go @@ -211,7 +211,7 @@ func TestResharderOneRefTable(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -263,7 +263,7 @@ func TestReshardStopFlags(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -314,7 +314,7 @@ func TestResharderOneRefStream(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -391,7 +391,7 @@ func TestResharderNoRefStream(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -604,7 +604,7 @@ func TestResharderUnnamedStream(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -652,7 +652,7 @@ func TestResharderMismatchedRefStreams(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -768,7 +768,7 @@ func TestResharderMixedTablesOrder1(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } @@ -835,7 +835,7 @@ func TestResharderMixedTablesOrder2(t *testing.T) { }, }, } - if err := env.wr.ts.ValidateAndSaveVSchema(context.Background(), env.keyspace, vs); err != nil { + if err := env.wr.ts.SaveVSchema(context.Background(), env.keyspace, vs); err != nil { t.Fatal(err) } diff --git a/go/vt/wrangler/traffic_switcher.go b/go/vt/wrangler/traffic_switcher.go index d000388faf1..c9bbb539807 100644 --- a/go/vt/wrangler/traffic_switcher.go +++ b/go/vt/wrangler/traffic_switcher.go @@ -1712,7 +1712,7 @@ func (ts *trafficSwitcher) dropParticipatingTablesFromKeyspace(ctx context.Conte for _, tableName := range ts.Tables() { delete(vschema.Tables, tableName) } - return ts.TopoServer().ValidateAndSaveVSchema(ctx, keyspace, vschema) + return ts.TopoServer().SaveVSchema(ctx, keyspace, vschema) } // FIXME: even after dropSourceShards there are still entries in the topo, need to research and fix @@ -1866,5 +1866,5 @@ func (ts *trafficSwitcher) addParticipatingTablesToKeyspace(ctx context.Context, vschema.Tables[table] = &vschemapb.Table{} } } - return ts.TopoServer().ValidateAndSaveVSchema(ctx, keyspace, vschema) + return ts.TopoServer().SaveVSchema(ctx, keyspace, vschema) } diff --git a/go/vt/wrangler/traffic_switcher_env_test.go b/go/vt/wrangler/traffic_switcher_env_test.go index 2b4d565d4df..7b219e47eec 100644 --- a/go/vt/wrangler/traffic_switcher_env_test.go +++ b/go/vt/wrangler/traffic_switcher_env_test.go @@ -161,12 +161,12 @@ func newTestTableMigraterCustom(ctx context.Context, t *testing.T, sourceShards, }, } if len(sourceShards) != 1 { - if err := tme.ts.ValidateAndSaveVSchema(ctx, "ks1", vs); err != nil { + if err := tme.ts.SaveVSchema(ctx, "ks1", vs); err != nil { t.Fatal(err) } } if len(targetShards) != 1 { - if err := tme.ts.ValidateAndSaveVSchema(ctx, "ks2", vs); err != nil { + if err := tme.ts.SaveVSchema(ctx, "ks2", vs); err != nil { t.Fatal(err) } } @@ -329,7 +329,7 @@ func newTestShardMigrater(ctx context.Context, t *testing.T, sourceShards, targe }, }, } - if err := tme.ts.ValidateAndSaveVSchema(ctx, "ks", vs); err != nil { + if err := tme.ts.SaveVSchema(ctx, "ks", vs); err != nil { t.Fatal(err) } if err := tme.ts.RebuildSrvVSchema(ctx, nil); err != nil { From 1d31e94ddf1e9a893f0545e6104f6fbd2af4194d Mon Sep 17 00:00:00 2001 From: Max Englander Date: Tue, 27 Jun 2023 18:32:48 -0400 Subject: [PATCH 6/7] use mutliline strings Co-authored-by: Andres Taylor Signed-off-by: Max Englander --- go/vt/vtctl/vtctl_test.go | 51 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/go/vt/vtctl/vtctl_test.go b/go/vt/vtctl/vtctl_test.go index 309363540b1..5424c124382 100644 --- a/go/vt/vtctl/vtctl_test.go +++ b/go/vt/vtctl/vtctl_test.go @@ -66,12 +66,59 @@ func TestApplyVSchema(t *testing.T) { { name: "UnknownParamsLogged", args: []string{"--vschema", unknownParamsLoggedVSchema, ks}, - want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W.* .* vtctl.go:.* Unknown param in vindex binary_vdx: hello\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: hello", + want: `/New VSchema object: +{ + "sharded": true, + "vindexes": { + "binary_vdx": { + "type": "binary", + "params": { + "hello": "world" + } + }, + "hash_vdx": { + "type": "hash", + "params": { + "foo": "bar", + "hello": "world" + } + } + } +} +If this is not what you expected, check the input data \(as JSON parsing will skip unexpected fields\)\. + +.*W.* .* vtctl.go:.* Unknown param in vindex binary_vdx: hello +W.* .* vtctl.go:.* Unknown param in vindex hash_vdx: foo +W.* .* vtctl.go:.* Unknown param in vindex hash_vdx: hello`, }, { name: "UnknownParamsLoggedWithDryRun", args: []string{"--vschema", unknownParamsLoggedDryRunVSchema, "--dry-run", ks}, - want: "/New VSchema object:\n{\n \"sharded\": true,\n \"vindexes\": {\n \"binary_vdx\": {\n \"type\": \"binary\",\n \"params\": {\n \"hello\": \"world\"\n }\n },\n \"hash_vdx\": {\n \"type\": \"hash\",\n \"params\": {\n \"foo\": \"bar\",\n \"hello\": \"world\"\n }\n }\n }\n}\nIf this is not what you expected, check the input data \\(as JSON parsing will skip unexpected fields\\)\\.\n\n.*W.* .* vtctl.go:.* Unknown param in vindex binary_vdx: hello\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: foo\nW.* .* vtctl.go:.* Unknown param in vindex hash_vdx: hello\nDry run: Skipping update of VSchema", + want: `/New VSchema object: +{ + "sharded": true, + "vindexes": { + "binary_vdx": { + "type": "binary", + "params": { + "hello": "world" + } + }, + "hash_vdx": { + "type": "hash", + "params": { + "foo": "bar", + "hello": "world" + } + } + } +} +If this is not what you expected, check the input data \(as JSON parsing will skip unexpected fields\)\. + +.*W.* .* vtctl.go:.* Unknown param in vindex binary_vdx: hello +W.* .* vtctl.go:.* Unknown param in vindex hash_vdx: foo +W.* .* vtctl.go:.* Unknown param in vindex hash_vdx: hello +Dry run: Skipping update of VSchema`, }, } for _, tt := range tests { From 755b19866065801a5654f96bd2f73730af2457a3 Mon Sep 17 00:00:00 2001 From: Max Englander Date: Wed, 28 Jun 2023 00:58:23 -0400 Subject: [PATCH 7/7] change metric name Signed-off-by: Max Englander --- changelog/18.0/18.0.0/summary.md | 6 +++--- go/vt/vtgate/vtgate.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/changelog/18.0/18.0.0/summary.md b/changelog/18.0/18.0.0/summary.md index 85e20fa1f5a..66202167d43 100644 --- a/changelog/18.0/18.0.0/summary.md +++ b/changelog/18.0/18.0.0/summary.md @@ -12,7 +12,7 @@ - [Deleted `k8stopo`](#deleted-k8stopo) - [Deleted `vtgr`](#deleted-vtgr) - **[New stats](#new-stats)** - - [VTGate Vindex unknown params](#vtgate-vindex-unknown-params) + - [VTGate Vindex unknown parameters](#vtgate-vindex-unknown-parameters) ## Major Changes @@ -44,6 +44,6 @@ The `vtgr` has been deprecated in Vitess 17, also see https://github.com/vitessi ### New stats -#### VTGate Vindex unknown params +#### VTGate Vindex unknown parameters -The VTGate stat `VindexUnknownParams` gauges unknown Vindex params found in the latest VSchema pulled from the topology. +The VTGate stat `VindexUnknownParameters` gauges unknown Vindex parameters found in the latest VSchema pulled from the topology. diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go index f15bde6b057..8f716385ff5 100644 --- a/go/vt/vtgate/vtgate.go +++ b/go/vt/vtgate/vtgate.go @@ -187,7 +187,7 @@ var ( vstreamSkewDelayCount = stats.NewCounter("VStreamEventsDelayedBySkewAlignment", "Number of events that had to wait because the skew across shards was too high") - vindexUnknownParams = stats.NewGauge("VindexUnknownParams", "Number of params unrecognized by Vindexes") + vindexUnknownParams = stats.NewGauge("VindexUnknownParameters", "Number of parameterss unrecognized by Vindexes") ) // VTGate is the rpc interface to vtgate. Only one instance