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 <mattalord@gmail.com> * Add unit test as well Signed-off-by: Matt Lord <mattalord@gmail.com> * Minor changes after review Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com>
vitessio · Jun 9, 2023 · c43d8e5 · c43d8e5
1 parent 122b4e4
commit c43d8e5
Show file tree

Hide file tree

Showing 4 changed files with 98 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,