From f98d261cd4741ec3feab0899bfd82892782293ca Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 20 Aug 2024 14:28:43 -0400 Subject: [PATCH] Changes after self review Signed-off-by: Matt Lord --- .../tabletmanager/vdiff/framework_test.go | 6 ++-- go/vt/vttablet/tabletmanager/vdiff/report.go | 12 +++---- .../tabletmanager/vdiff/report_test.go | 33 +++++++++---------- .../tabletmanager/vdiff/table_differ.go | 20 +++++------ .../vdiff/workflow_differ_test.go | 2 +- 5 files changed, 35 insertions(+), 38 deletions(-) diff --git a/go/vt/vttablet/tabletmanager/vdiff/framework_test.go b/go/vt/vttablet/tabletmanager/vdiff/framework_test.go index 312cf444e7d..e02498d3c5d 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/framework_test.go +++ b/go/vt/vttablet/tabletmanager/vdiff/framework_test.go @@ -662,14 +662,14 @@ func (tvde *testVDiffEnv) addTablet(id int, keyspace, shard string, tabletType t return tvde.tablets[id] } -func (tvde *testVDiffEnv) createController(t *testing.T) *controller { +func (tvde *testVDiffEnv) createController(t *testing.T, id int) *controller { controllerQR := sqltypes.MakeTestResult(sqltypes.MakeTestFields( vdiffTestCols, vdiffTestColTypes, ), - fmt.Sprintf("1|%s|%s|%s|%s|%s|%s|%s|", uuid.New(), tvde.workflow, tstenv.KeyspaceName, tstenv.ShardName, vdiffDBName, PendingState, optionsJS), + fmt.Sprintf("%d|%s|%s|%s|%s|%s|%s|%s|", id, uuid.New(), tvde.workflow, tstenv.KeyspaceName, tstenv.ShardName, vdiffDBName, PendingState, optionsJS), ) - tvde.dbClient.ExpectRequest("select * from _vt.vdiff where id = 1", noResults, nil) + tvde.dbClient.ExpectRequest(fmt.Sprintf("select * from _vt.vdiff where id = %d", id), noResults, nil) ct, err := newController(context.Background(), controllerQR.Named().Row(), tvde.dbClientFactory, tstenv.TopoServ, tvde.vde, tvde.opts) require.NoError(t, err) return ct diff --git a/go/vt/vttablet/tabletmanager/vdiff/report.go b/go/vt/vttablet/tabletmanager/vdiff/report.go index 0171836f55b..cac7560fc4c 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/report.go +++ b/go/vt/vttablet/tabletmanager/vdiff/report.go @@ -63,7 +63,7 @@ type RowDiff struct { Query string `json:"Query,omitempty"` } -func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, reportOptions *tabletmanagerdatapb.VDiffReportOptions) (*RowDiff, error) { +func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, opts *tabletmanagerdatapb.VDiffReportOptions) (*RowDiff, error) { rd := &RowDiff{} rd.Row = make(map[string]string) statement, err := td.wd.ct.vde.parser.Parse(queryStmt) @@ -75,8 +75,8 @@ func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, report return nil, fmt.Errorf("unexpected: %+v", sqlparser.String(statement)) } - if reportOptions.GetDebugQuery() { - rd.Query = td.genDebugQueryDiff(sel, row, reportOptions.GetOnlyPks()) + if opts.GetDebugQuery() { + rd.Query = td.genDebugQueryDiff(sel, row, opts.GetOnlyPks()) } addVal := func(index int, truncateAt int) { @@ -101,14 +101,14 @@ func (td *tableDiffer) genRowDiff(queryStmt string, row []sqltypes.Value, report pks[pkI] = struct{}{} } - if reportOptions.GetOnlyPks() { + if opts.GetOnlyPks() { return rd, nil } - rowDiffColumnTruncateAt := int(reportOptions.GetRowDiffColumnTruncateAt()) + truncateAt := int(opts.GetRowDiffColumnTruncateAt()) for i := range sel.SelectExprs { if _, pk := pks[i]; !pk { - addVal(i, rowDiffColumnTruncateAt) + addVal(i, truncateAt) } } diff --git a/go/vt/vttablet/tabletmanager/vdiff/report_test.go b/go/vt/vttablet/tabletmanager/vdiff/report_test.go index 39b3158b9e3..9b6d840f751 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/report_test.go +++ b/go/vt/vttablet/tabletmanager/vdiff/report_test.go @@ -1,5 +1,5 @@ /* -Copyright 2022 The Vitess Authors. +Copyright 2024 The Vitess Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -40,7 +40,6 @@ func TestGenRowDiff(t *testing.T) { row []sqltypes.Value reportOptions *tabletmanagerdatapb.VDiffReportOptions want *RowDiff - wantErr bool }{ { name: "defaults", @@ -48,24 +47,27 @@ func TestGenRowDiff(t *testing.T) { TableDefinitions: []*tabletmanagerdatapb.TableDefinition{ { Name: "t1", - Columns: []string{"c1", "c2"}, - PrimaryKeyColumns: []string{"c1"}, - Fields: sqltypes.MakeTestFields("c1|c2", "int64|int64"), + Columns: []string{"c1", "c2", "c3", "c4", "c5"}, + PrimaryKeyColumns: []string{"c1", "c5"}, + Fields: sqltypes.MakeTestFields("c1|c2|c3|c4|c5", "int64|int64|varchar|varchar|int64"), }, }, }, - query: "select c1,c2 from t1", + query: "select c1,c2,c3,c4,c5 from t1", tablePlan: &tablePlan{ - selectPks: []int{0}, + selectPks: []int{0, 4}, }, row: []sqltypes.Value{ sqltypes.NewInt64(1), sqltypes.NewInt64(2), + sqltypes.NewVarChar("hi3"), + sqltypes.NewVarChar("hi4"), + sqltypes.NewInt64(5), }, reportOptions: &tabletmanagerdatapb.VDiffReportOptions{}, want: &RowDiff{ - Row: map[string]string{ - "c1": "1", "c2": "2", + Row: map[string]string{ // The two PK cols should be first + "c1": "1", "c5": "5", "c2": "2", "c3": "hi3", "c4": "hi4", }, }, }, @@ -167,23 +169,18 @@ func TestGenRowDiff(t *testing.T) { require.NotNil(t, tc.reportOptions) vdenv.tmc.schema = tc.schema - ct := vdenv.createController(t) + ct := vdenv.createController(t, 1) wd, err := newWorkflowDiffer(ct, vdenv.opts, collations.MySQL8()) require.NoError(t, err) - td := &tableDiffer{ wd: wd, sourceQuery: tc.query, tablePlan: tc.tablePlan, } + got, err := td.genRowDiff(tc.query, tc.row, tc.reportOptions) - if tc.wantErr { - require.Error(t, err, "tableDiffer.genRowDiff() error = %v, wantErr %v", - err, tc.wantErr) - return - } else { - require.EqualValues(t, tc.want, got) - } + require.NoError(t, err) + require.EqualValues(t, tc.want, got) }) } } diff --git a/go/vt/vttablet/tabletmanager/vdiff/table_differ.go b/go/vt/vttablet/tabletmanager/vdiff/table_differ.go index 0588cece9d9..102d7535af9 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/table_differ.go +++ b/go/vt/vttablet/tabletmanager/vdiff/table_differ.go @@ -486,7 +486,7 @@ func (td *tableDiffer) setupRowSorters() { } } -func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatapb.VDiffCoreOptions, reportOptions *tabletmanagerdatapb.VDiffReportOptions, stop <-chan time.Time) (*DiffReport, error) { +func (td *tableDiffer) diff(ctx context.Context, coreOpts *tabletmanagerdatapb.VDiffCoreOptions, reportOpts *tabletmanagerdatapb.VDiffReportOptions, stop <-chan time.Time) (*DiffReport, error) { defer td.wd.ct.TableDiffPhaseTimings.Record(fmt.Sprintf("%s.%s", td.table.Name, diffingTable), time.Now()) dbClient := td.wd.ct.dbClientFactory() if err := dbClient.Connect(); err != nil { @@ -539,9 +539,9 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap globalStats.RowsDiffedCount.Add(dr.ProcessedRows) }() - rowsToCompare := coreOptions.GetMaxRows() - maxExtraRowsToCompare := coreOptions.GetMaxExtraRowsToCompare() - maxReportSampleRows := reportOptions.GetMaxSampleRows() + rowsToCompare := coreOpts.GetMaxRows() + maxExtraRowsToCompare := coreOpts.GetMaxExtraRowsToCompare() + maxReportSampleRows := reportOpts.GetMaxSampleRows() for { lastProcessedRow = sourceRow @@ -592,7 +592,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap advanceSource = true advanceTarget = true if sourceRow == nil { - diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, targetRow, reportOptions) + diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, targetRow, reportOpts) if err != nil { return nil, vterrors.Wrap(err, "unexpected error generating diff") } @@ -610,7 +610,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap if targetRow == nil { // No more rows from the target but we know we have more rows from // source, so drain them and update the counts. - diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOptions) + diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOpts) if err != nil { return nil, vterrors.Wrap(err, "unexpected error generating diff") } @@ -633,7 +633,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap return nil, err case c < 0: if dr.ExtraRowsSource < maxExtraRowsToCompare { - diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOptions) + diffRow, err := td.genRowDiff(td.tablePlan.sourceQuery, sourceRow, reportOpts) if err != nil { return nil, vterrors.Wrap(err, "unexpected error generating diff") } @@ -644,7 +644,7 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap continue case c > 0: if dr.ExtraRowsTarget < maxExtraRowsToCompare { - diffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOptions) + diffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOpts) if err != nil { return nil, vterrors.Wrap(err, "unexpected error generating diff") } @@ -664,11 +664,11 @@ func (td *tableDiffer) diff(ctx context.Context, coreOptions *tabletmanagerdatap case c != 0: // We don't do a second pass to compare mismatched rows so we can cap the slice here. if maxReportSampleRows == 0 || dr.MismatchedRows < maxReportSampleRows { - sourceDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, sourceRow, reportOptions) + sourceDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, sourceRow, reportOpts) if err != nil { return nil, vterrors.Wrap(err, "unexpected error generating diff") } - targetDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOptions) + targetDiffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOpts) if err != nil { return nil, vterrors.Wrap(err, "unexpected error generating diff") } diff --git a/go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go b/go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go index 928d595e0cf..0054c37faf7 100644 --- a/go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go +++ b/go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go @@ -588,7 +588,7 @@ func TestBuildPlanInclude(t *testing.T) { vdenv := newTestVDiffEnv(t) defer vdenv.close() - ct := vdenv.createController(t) + ct := vdenv.createController(t, 1) schm := &tabletmanagerdatapb.SchemaDefinition{ TableDefinitions: []*tabletmanagerdatapb.TableDefinition{{