From c43d8e578aba6ffb2ce731d30f07ce6c8de80199 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 9 Jun 2023 12:30:33 -0400 Subject: [PATCH] VReplication: Fix VDiff2 DeleteByUUID Query (#13255) * Fix VDiff2 DeleteByUUID Query It was incorrect and thus deleting every row in the vdiff table. IIRC, this was a mistake on my part as I was first using a WHERE based query and then had to move to LEFT JOIN usage as there may not always be vdiff_table records. When doing so, however, I left the last AND clause in place when it should have become the sole predicate in a new WHERE clause after the LEFT JOIN condition. Signed-off-by: Matt Lord * Add unit test as well Signed-off-by: Matt Lord * Minor changes after review Signed-off-by: Matt Lord --------- Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/vdiff2_test.go | 42 +++++++++++++-- .../vreplication/vdiff_helper_test.go | 27 +++++----- .../tabletmanager/vdiff/action_test.go | 53 ++++++++++++++++--- go/vt/vttablet/tabletmanager/vdiff/schema.go | 2 +- 4 files changed, 98 insertions(+), 26 deletions(-) diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index f6885021d9b..e42cd6b73fe 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -23,6 +23,7 @@ import ( "time" "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" "vitess.io/vitess/go/test/endtoend/cluster" "vitess.io/vitess/go/vt/sqlparser" @@ -234,16 +235,47 @@ func testCLIErrors(t *testing.T, ksWorkflow, cells string) { func testDelete(t *testing.T, ksWorkflow, cells string) { t.Run("Delete", func(t *testing.T) { - // test show verbose too as a side effect + // Let's be sure that we have at least 3 unique VDiffs. + // We have one record in the SHOW output per VDiff, per + // shard. So we want to get a count of the unique VDiffs + // by UUID. + uuidCount := func(uuids []gjson.Result) int64 { + seen := make(map[string]struct{}) + for _, uuid := range uuids { + seen[uuid.String()] = struct{}{} + } + return int64(len(seen)) + } + _, output := performVDiff2Action(t, ksWorkflow, cells, "show", "all", false) + initialVDiffCount := uuidCount(gjson.Get(output, "#.UUID").Array()) + for ; initialVDiffCount < 3; initialVDiffCount++ { + _, _ = performVDiff2Action(t, ksWorkflow, cells, "create", "", false) + } + + // Now let's confirm that we have at least 3 unique VDiffs. + _, output = performVDiff2Action(t, ksWorkflow, cells, "show", "all", false) + require.GreaterOrEqual(t, uuidCount(gjson.Get(output, "#.UUID").Array()), int64(3)) + // And that our initial count is what we expect. + require.Equal(t, initialVDiffCount, uuidCount(gjson.Get(output, "#.UUID").Array())) + + // Test show last with verbose too as a side effect. uuid, output := performVDiff2Action(t, ksWorkflow, cells, "show", "last", false, "--verbose") - // only present with --verbose + // The TableSummary is only present with --verbose. require.Contains(t, output, `"TableSummary":`) + + // Now let's delete one of the VDiffs. _, output = performVDiff2Action(t, ksWorkflow, cells, "delete", uuid, false) - require.Contains(t, output, `"Status": "completed"`) + require.Equal(t, "completed", gjson.Get(output, "Status").String()) + // And confirm that our unique VDiff count has only decreased by one. + _, output = performVDiff2Action(t, ksWorkflow, cells, "show", "all", false) + require.Equal(t, initialVDiffCount-1, uuidCount(gjson.Get(output, "#.UUID").Array())) + + // Now let's delete all of them. _, output = performVDiff2Action(t, ksWorkflow, cells, "delete", "all", false) - require.Contains(t, output, `"Status": "completed"`) + require.Equal(t, "completed", gjson.Get(output, "Status").String()) + // And finally confirm that we have no more VDiffs. _, output = performVDiff2Action(t, ksWorkflow, cells, "show", "all", false) - require.Equal(t, "[]\n", output) + require.Equal(t, int64(0), gjson.Get(output, "#").Int()) }) } diff --git a/go/test/endtoend/vreplication/vdiff_helper_test.go b/go/test/endtoend/vreplication/vdiff_helper_test.go index 5beab7aa19a..7c8b33650db 100644 --- a/go/test/endtoend/vreplication/vdiff_helper_test.go +++ b/go/test/endtoend/vreplication/vdiff_helper_test.go @@ -23,8 +23,8 @@ import ( "testing" "time" - "github.com/buger/jsonparser" "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" "vitess.io/vitess/go/sqlescape" "vitess.io/vitess/go/sqltypes" @@ -176,7 +176,7 @@ func performVDiff2Action(t *testing.T, ksWorkflow, cells, action, actionArg stri log.Infof("vdiff2 output: %+v (err: %+v)", output, err) if !expectError { require.Nil(t, err) - uuid, err = jsonparser.GetString([]byte(output), "UUID") + uuid = gjson.Get(output, "UUID").String() if action != "delete" && !(action == "show" && actionArg == "all") { // a UUID is not required require.NoError(t, err) require.NotEmpty(t, uuid) @@ -195,19 +195,18 @@ type vdiffInfo struct { Progress vdiff2.ProgressReport } -func getVDiffInfo(jsonStr string) *vdiffInfo { +func getVDiffInfo(json string) *vdiffInfo { var info vdiffInfo - json := []byte(jsonStr) - info.Workflow, _ = jsonparser.GetString(json, "Workflow") - info.Keyspace, _ = jsonparser.GetString(json, "Keyspace") - info.State, _ = jsonparser.GetString(json, "State") - info.Shards, _ = jsonparser.GetString(json, "Shards") - info.RowsCompared, _ = jsonparser.GetInt(json, "RowsCompared") - info.StartedAt, _ = jsonparser.GetString(json, "StartedAt") - info.CompletedAt, _ = jsonparser.GetString(json, "CompletedAt") - info.HasMismatch, _ = jsonparser.GetBoolean(json, "HasMismatch") - info.Progress.Percentage, _ = jsonparser.GetFloat(json, "Progress", "Percentage") - info.Progress.ETA, _ = jsonparser.GetString(json, "Progress", "ETA") + info.Workflow = gjson.Get(json, "Workflow").String() + info.Keyspace = gjson.Get(json, "Keyspace").String() + info.State = gjson.Get(json, "State").String() + info.Shards = gjson.Get(json, "Shards").String() + info.RowsCompared = gjson.Get(json, "RowsCompared").Int() + info.StartedAt = gjson.Get(json, "StartedAt").String() + info.CompletedAt = gjson.Get(json, "CompletedAt").String() + info.HasMismatch = gjson.Get(json, "HasMismatch").Bool() + info.Progress.Percentage = gjson.Get(json, "Progress.Percentage").Float() + info.Progress.ETA = gjson.Get(json, "Progress.ETA").String() return &info } diff --git a/go/vt/vttablet/tabletmanager/vdiff/action_test.go b/go/vt/vttablet/tabletmanager/vdiff/action_test.go index 7c06f5b6f2f..b6ad3d65775 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/action_test.go +++ b/go/vt/vttablet/tabletmanager/vdiff/action_test.go @@ -18,10 +18,14 @@ package vdiff import ( "context" + "fmt" "reflect" "testing" "time" + "github.com/google/uuid" + + "vitess.io/vitess/go/sqltypes" tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/vterrors" @@ -30,27 +34,64 @@ import ( func TestPerformVDiffAction(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() + vdiffenv := newTestVDiffEnv(t) + defer vdiffenv.close() + keyspace := "ks" + workflow := "wf" + uuid := uuid.New().String() tests := []struct { - name string - vde *Engine - req *tabletmanagerdatapb.VDiffRequest - want *tabletmanagerdatapb.VDiffResponse - wantErr error + name string + vde *Engine + req *tabletmanagerdatapb.VDiffRequest + want *tabletmanagerdatapb.VDiffResponse + expectQueries []string + wantErr error }{ { name: "engine not open", vde: &Engine{isOpen: false}, wantErr: vterrors.New(vtrpcpb.Code_UNAVAILABLE, "vdiff engine is closed"), }, + { + name: "delete by uuid", + req: &tabletmanagerdatapb.VDiffRequest{ + Action: string(DeleteAction), + ActionArg: uuid, + }, + expectQueries: []string{ + fmt.Sprintf(`delete from vd, vdt using _vt.vdiff as vd left join _vt.vdiff_table as vdt on (vd.id = vdt.vdiff_id) + where vd.vdiff_uuid = %s`, encodeString(uuid)), + }, + }, + { + name: "delete all", + req: &tabletmanagerdatapb.VDiffRequest{ + Action: string(DeleteAction), + ActionArg: "all", + Keyspace: keyspace, + Workflow: workflow, + }, + expectQueries: []string{ + fmt.Sprintf(`delete from vd, vdt, vdl using _vt.vdiff as vd left join _vt.vdiff_table as vdt on (vd.id = vdt.vdiff_id) + left join _vt.vdiff_log as vdl on (vd.id = vdl.vdiff_id) + where vd.keyspace = %s and vd.workflow = %s`, encodeString(keyspace), encodeString(workflow)), + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.vde == nil { + tt.vde = vdiffenv.vde + } + for _, query := range tt.expectQueries { + vdiffenv.dbClient.ExpectRequest(query, &sqltypes.Result{}, nil) + } got, err := tt.vde.PerformVDiffAction(ctx, tt.req) if tt.wantErr != nil && !vterrors.Equals(err, tt.wantErr) { t.Errorf("Engine.PerformVDiffAction() error = %v, wantErr %v", err, tt.wantErr) return } - if !reflect.DeepEqual(got, tt.want) { + if tt.want != nil && !reflect.DeepEqual(got, tt.want) { t.Errorf("Engine.PerformVDiffAction() = %v, want %v", got, tt.want) } }) diff --git a/go/vt/vttablet/tabletmanager/vdiff/schema.go b/go/vt/vttablet/tabletmanager/vdiff/schema.go index 8055f3471ac..f82f03106b4 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/schema.go +++ b/go/vt/vttablet/tabletmanager/vdiff/schema.go @@ -31,7 +31,7 @@ const ( left join _vt.vdiff_log as vdl on (vd.id = vdl.vdiff_id) where vd.keyspace = %s and vd.workflow = %s` sqlDeleteVDiffByUUID = `delete from vd, vdt using _vt.vdiff as vd left join _vt.vdiff_table as vdt on (vd.id = vdt.vdiff_id) - and vd.vdiff_uuid = %s` + where vd.vdiff_uuid = %s` sqlVDiffSummary = `select vd.state as vdiff_state, vd.last_error as last_error, vdt.table_name as table_name, vd.vdiff_uuid as 'uuid', vdt.state as table_state, vdt.table_rows as table_rows, vd.started_at as started_at, vdt.rows_compared as rows_compared, vd.completed_at as completed_at,