From fa732a8fff00947af4e2d6a3a380c0cdd3afa13a Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 14:24:34 -0500 Subject: [PATCH 01/10] Fix vtctldclient vdiff limit bug Signed-off-by: Matt Lord --- .../vtctldclient/command/vreplication/vdiff/vdiff.go | 12 ++++++------ go/vt/vtctl/workflow/server.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index 726da479b56..545c0229735 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -57,13 +57,13 @@ var ( TargetCells []string TabletTypes []topodatapb.TabletType Tables []string - Limit uint32 // We only accept positive values but pass on an int64 + Limit int64 FilteredReplicationWaitTime time.Duration DebugQuery bool - MaxReportSampleRows uint32 // We only accept positive values but pass on an int64 + MaxReportSampleRows int64 OnlyPKs bool UpdateTableStats bool - MaxExtraRowsToCompare uint32 // We only accept positive values but pass on an int64 + MaxExtraRowsToCompare int64 Wait bool WaitUpdateInterval time.Duration AutoRetry bool @@ -863,12 +863,12 @@ func registerCommands(root *cobra.Command) { create.Flags().Var((*topoprotopb.TabletTypeListFlag)(&createOptions.TabletTypes), "tablet-types", "Tablet types to use on the source and target.") create.Flags().BoolVar(&common.CreateOptions.TabletTypesInPreferenceOrder, "tablet-types-in-preference-order", true, "When performing source tablet selection, look for candidates in the type order as they are listed in the tablet-types flag.") create.Flags().DurationVar(&createOptions.FilteredReplicationWaitTime, "filtered-replication-wait-time", 30*time.Second, "Specifies the maximum time to wait, in seconds, for replication to catch up when syncing tablet streams.") - create.Flags().Uint32Var(&createOptions.Limit, "limit", math.MaxUint32, "Max rows to stop comparing after.") + create.Flags().Int64Var(&createOptions.Limit, "limit", math.MaxInt64, "Max rows to stop comparing after.") create.Flags().BoolVar(&createOptions.DebugQuery, "debug-query", false, "Adds a mysql query to the report that can be used for further debugging.") - create.Flags().Uint32Var(&createOptions.MaxReportSampleRows, "max-report-sample-rows", 10, "Maximum number of row differences to report (0 for all differences). NOTE: when increasing this value it is highly recommended to also specify --only-pks") + create.Flags().Int64Var(&createOptions.MaxReportSampleRows, "max-report-sample-rows", 10, "Maximum number of row differences to report (0 for all differences). NOTE: when increasing this value it is highly recommended to also specify --only-pks") create.Flags().BoolVar(&createOptions.OnlyPKs, "only-pks", false, "When reporting missing rows, only show primary keys in the report.") create.Flags().StringSliceVar(&createOptions.Tables, "tables", nil, "Only run vdiff for these tables in the workflow.") - create.Flags().Uint32Var(&createOptions.MaxExtraRowsToCompare, "max-extra-rows-to-compare", 1000, "If there are collation differences between the source and target, you can have rows that are identical but simply returned in a different order from MySQL. We will do a second pass to compare the rows for any actual differences in this case and this flag allows you to control the resources used for this operation.") + create.Flags().Int64Var(&createOptions.MaxExtraRowsToCompare, "max-extra-rows-to-compare", 1000, "If there are collation differences between the source and target, you can have rows that are identical but simply returned in a different order from MySQL. We will do a second pass to compare the rows for any actual differences in this case and this flag allows you to control the resources used for this operation.") create.Flags().BoolVar(&createOptions.Wait, "wait", false, "When creating or resuming a vdiff, wait for it to finish before exiting.") create.Flags().DurationVar(&createOptions.WaitUpdateInterval, "wait-update-interval", time.Duration(1*time.Minute), "When waiting on a vdiff to finish, check and display the current status this often.") create.Flags().BoolVar(&createOptions.AutoRetry, "auto-retry", true, "Should this vdiff automatically retry and continue in case of recoverable errors.") diff --git a/go/vt/vtctl/workflow/server.go b/go/vt/vtctl/workflow/server.go index d15e9dc1bbd..5302b33edb8 100644 --- a/go/vt/vtctl/workflow/server.go +++ b/go/vt/vtctl/workflow/server.go @@ -1632,7 +1632,7 @@ func (s *Server) VDiffCreate(ctx context.Context, req *vtctldatapb.VDiffCreateRe CoreOptions: &tabletmanagerdatapb.VDiffCoreOptions{ Tables: strings.Join(req.Tables, ","), AutoRetry: req.AutoRetry, - MaxRows: req.MaxExtraRowsToCompare, + MaxRows: req.Limit, TimeoutSeconds: req.FilteredReplicationWaitTime.Seconds, MaxExtraRowsToCompare: req.MaxExtraRowsToCompare, UpdateTableStats: req.UpdateTableStats, From bcef5b40127c5004e147c909d2f9e0d65930ab3d Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 16:57:47 -0500 Subject: [PATCH 02/10] Add e2e test for flag handling Signed-off-by: Matt Lord --- .../command/vreplication/vdiff/vdiff.go | 6 +- go/test/endtoend/vreplication/vdiff2_test.go | 103 ++++++++++++++---- go/vt/vtctl/grpcvtctldclient/client_gen.go | 18 +-- go/vt/vtctl/localvtctldclient/client_gen.go | 10 +- 4 files changed, 101 insertions(+), 36 deletions(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index 545c0229735..f512242510f 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -271,16 +271,16 @@ func commandCreate(cmd *cobra.Command, args []string) error { TabletTypes: createOptions.TabletTypes, TabletSelectionPreference: tsp, Tables: createOptions.Tables, - Limit: int64(createOptions.Limit), + Limit: createOptions.Limit, FilteredReplicationWaitTime: protoutil.DurationToProto(createOptions.FilteredReplicationWaitTime), DebugQuery: createOptions.DebugQuery, OnlyPKs: createOptions.OnlyPKs, UpdateTableStats: createOptions.UpdateTableStats, - MaxExtraRowsToCompare: int64(createOptions.MaxExtraRowsToCompare), + MaxExtraRowsToCompare: createOptions.MaxExtraRowsToCompare, Wait: createOptions.Wait, WaitUpdateInterval: protoutil.DurationToProto(createOptions.WaitUpdateInterval), AutoRetry: createOptions.AutoRetry, - MaxReportSampleRows: int64(createOptions.MaxReportSampleRows), + MaxReportSampleRows: createOptions.MaxReportSampleRows, }) if err != nil { diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index eb96985af57..a6c75da1b19 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -22,8 +22,10 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/tidwall/gjson" + "golang.org/x/exp/maps" "vitess.io/vitess/go/test/endtoend/cluster" "vitess.io/vitess/go/vt/sqlparser" @@ -40,10 +42,11 @@ type testCase struct { retryInsert string resume bool // test resume functionality with this workflow // If testing resume, what new rows should be diff'd. These rows must have a PK > all initial rows and retry rows. - resumeInsert string - stop bool // test stop functionality with this workflow - testCLIErrors bool // test CLI errors against this workflow (only needs to be done once) - testCLICreateWait bool // test CLI create and wait until done against this workflow (only needs to be done once) + resumeInsert string + stop bool // test stop functionality with this workflow + testCLIErrors bool // test CLI errors against this workflow (only needs to be done once) + testCLICreateWait bool // test CLI create and wait until done against this workflow (only needs to be done once) + testCLIFlagHandling bool } const ( @@ -55,21 +58,22 @@ const ( var testCases = []*testCase{ { - name: "MoveTables/unsharded to two shards", - workflow: "p1c2", - typ: "MoveTables", - sourceKs: "product", - targetKs: "customer", - sourceShards: "0", - targetShards: "-80,80-", - tabletBaseID: 200, - tables: "customer,Lead,Lead-1", - autoRetryError: true, - retryInsert: `insert into customer(cid, name, typ) values(1991234, 'Testy McTester', 'soho')`, - resume: true, - resumeInsert: `insert into customer(cid, name, typ) values(1992234, 'Testy McTester (redux)', 'enterprise')`, - testCLIErrors: true, // test for errors in the simplest workflow - testCLICreateWait: true, // test wait on create feature against simplest workflow + name: "MoveTables/unsharded to two shards", + workflow: "p1c2", + typ: "MoveTables", + sourceKs: "product", + targetKs: "customer", + sourceShards: "0", + targetShards: "-80,80-", + tabletBaseID: 200, + tables: "customer,Lead,Lead-1", + autoRetryError: true, + retryInsert: `insert into customer(cid, name, typ) values(1991234, 'Testy McTester', 'soho')`, + resume: true, + resumeInsert: `insert into customer(cid, name, typ) values(1992234, 'Testy McTester (redux)', 'enterprise')`, + testCLIErrors: true, // test for errors in the simplest workflow + testCLICreateWait: true, // test wait on create feature against simplest workflow + testCLIFlagHandling: true, }, { name: "Reshard Merge/split 2 to 3", @@ -207,6 +211,9 @@ func testWorkflow(t *testing.T, vc *VitessCluster, tc *testCase, tks *Keyspace, if tc.testCLIErrors { testCLIErrors(t, ksWorkflow, allCellNames) } + if tc.testCLIFlagHandling { + testCLIFlagHandling(t, tc.targetKs, tc.workflow, cells[0]) + } testDelete(t, ksWorkflow, allCellNames) @@ -242,6 +249,64 @@ func testCLIErrors(t *testing.T, ksWorkflow, cells string) { }) } +// testCLIFlagHandling tests that the vtctldclient CLI flags are handled correctly +// from vtctldclient->vtctld->vttablet->mysqld. +func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell) { + // Keys are in the tabletmanagerdata.VDiff*Options proto message definitions. + coreOpts := map[string]string{ + "max_rows": "999", + "max_extra_rows_to_compare": "777", + "auto_retry": "true", + "update_table_stats": "true", + "timeout_seconds": "60", + } + pickerOpts := map[string]string{ + "source_cell": "zone1,zone2,zone3,zonefoo", + "target_cell": "zone1,zone2,zone3,zonefoo", + "tablet_types": "replica,primary,rdonly", + } + reportOpts := map[string]string{ + "max_sample_rows": "888", + "only_pks": "true", + } + + t.Run("Client flag handling", func(t *testing.T) { + res, err := vc.VtctldClient.ExecuteCommandWithOutput("vdiff", "--target-keyspace", targetKs, "--workflow", workflowName, + "create", "--limit", coreOpts["max_rows"], "--max-report-sample-rows", reportOpts["max_sample_rows"], + "--max-extra-rows-to-compare", coreOpts["max_extra_rows_to_compare"], "--filtered-replication-wait-time", + coreOpts["timeout_seconds"]+"s", "--source-cells", pickerOpts["source_cell"], "--target-cells", + pickerOpts["target_cell"], "--tablet-types", pickerOpts["tablet_types"], + fmt.Sprintf("--update-table-stats=%s", coreOpts["update_table_stats"]), + fmt.Sprintf("--auto-retry=%s", coreOpts["auto_retry"]), fmt.Sprintf("--only-pks=%s", reportOpts["only_pks"]), + "--tablet-types-in-preference-order=false", "--format=json") + require.NoError(t, err, "vdiff command failed: %s", res) + jsonRes := gjson.Parse(res) + vdid := jsonRes.Get("UUID").String() + _, err = uuid.Parse(vdid) + require.NoError(t, err, "invalid UUID: %s", vdid) + + // Confirm that the options were set and saved correctly. + query := sqlparser.BuildParsedQuery("select options from %s.vdiff where vdiff_uuid = %s", + sidecarDBIdentifier, encodeString(vdid)).Query + tablets := vc.getVttabletsInKeyspace(t, cell, targetKs, "PRIMARY") + require.Greater(t, len(tablets), 0, "no primary tablets found in keyspace %s", targetKs) + tablet := maps.Values(tablets)[0] + qres, err := tablet.QueryTablet(query, targetKs, false) + require.NoError(t, err, "query failed: %s", query) + require.NotNil(t, qres, "query returned nil result: %s", query) + jsonRes = gjson.Parse(qres.Rows[0][0].ToString()) + for key, val := range coreOpts { + require.Equal(t, val, jsonRes.Get("core_options."+key).String(), "unexpected value for key core_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("core_options."+key).String()) + } + for key, val := range pickerOpts { + require.Equal(t, val, jsonRes.Get("picker_options."+key).String(), "unexpected value for key picker_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("picker_options."+key).String()) + } + for key, val := range reportOpts { + require.Equal(t, val, jsonRes.Get("report_options."+key).String(), "unexpected value for key report_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("report_options."+key).String()) + } + }) +} + func testDelete(t *testing.T, ksWorkflow, cells string) { t.Run("Delete", func(t *testing.T) { // Let's be sure that we have at least 3 unique VDiffs. diff --git a/go/vt/vtctl/grpcvtctldclient/client_gen.go b/go/vt/vtctl/grpcvtctldclient/client_gen.go index 4020afa3307..c1d251487fd 100644 --- a/go/vt/vtctl/grpcvtctldclient/client_gen.go +++ b/go/vt/vtctl/grpcvtctldclient/client_gen.go @@ -128,15 +128,6 @@ func (client *gRPCVtctldClient) CleanupSchemaMigration(ctx context.Context, in * return client.c.CleanupSchemaMigration(ctx, in, opts...) } -// ForceCutOverSchemaMigration is part of the vtctlservicepb.VtctldClient interface. -func (client *gRPCVtctldClient) ForceCutOverSchemaMigration(ctx context.Context, in *vtctldatapb.ForceCutOverSchemaMigrationRequest, opts ...grpc.CallOption) (*vtctldatapb.ForceCutOverSchemaMigrationResponse, error) { - if client.c == nil { - return nil, status.Error(codes.Unavailable, connClosedMsg) - } - - return client.c.ForceCutOverSchemaMigration(ctx, in, opts...) -} - // CompleteSchemaMigration is part of the vtctlservicepb.VtctldClient interface. func (client *gRPCVtctldClient) CompleteSchemaMigration(ctx context.Context, in *vtctldatapb.CompleteSchemaMigrationRequest, opts ...grpc.CallOption) (*vtctldatapb.CompleteSchemaMigrationResponse, error) { if client.c == nil { @@ -263,6 +254,15 @@ func (client *gRPCVtctldClient) FindAllShardsInKeyspace(ctx context.Context, in return client.c.FindAllShardsInKeyspace(ctx, in, opts...) } +// ForceCutOverSchemaMigration is part of the vtctlservicepb.VtctldClient interface. +func (client *gRPCVtctldClient) ForceCutOverSchemaMigration(ctx context.Context, in *vtctldatapb.ForceCutOverSchemaMigrationRequest, opts ...grpc.CallOption) (*vtctldatapb.ForceCutOverSchemaMigrationResponse, error) { + if client.c == nil { + return nil, status.Error(codes.Unavailable, connClosedMsg) + } + + return client.c.ForceCutOverSchemaMigration(ctx, in, opts...) +} + // GetBackups is part of the vtctlservicepb.VtctldClient interface. func (client *gRPCVtctldClient) GetBackups(ctx context.Context, in *vtctldatapb.GetBackupsRequest, opts ...grpc.CallOption) (*vtctldatapb.GetBackupsResponse, error) { if client.c == nil { diff --git a/go/vt/vtctl/localvtctldclient/client_gen.go b/go/vt/vtctl/localvtctldclient/client_gen.go index 019ce619054..cbde68f1b27 100644 --- a/go/vt/vtctl/localvtctldclient/client_gen.go +++ b/go/vt/vtctl/localvtctldclient/client_gen.go @@ -176,11 +176,6 @@ func (client *localVtctldClient) CleanupSchemaMigration(ctx context.Context, in return client.s.CleanupSchemaMigration(ctx, in) } -// ForceCutOverSchemaMigration is part of the vtctlservicepb.VtctldClient interface. -func (client *localVtctldClient) ForceCutOverSchemaMigration(ctx context.Context, in *vtctldatapb.ForceCutOverSchemaMigrationRequest, opts ...grpc.CallOption) (*vtctldatapb.ForceCutOverSchemaMigrationResponse, error) { - return client.s.ForceCutOverSchemaMigration(ctx, in) -} - // CompleteSchemaMigration is part of the vtctlservicepb.VtctldClient interface. func (client *localVtctldClient) CompleteSchemaMigration(ctx context.Context, in *vtctldatapb.CompleteSchemaMigrationRequest, opts ...grpc.CallOption) (*vtctldatapb.CompleteSchemaMigrationResponse, error) { return client.s.CompleteSchemaMigration(ctx, in) @@ -251,6 +246,11 @@ func (client *localVtctldClient) FindAllShardsInKeyspace(ctx context.Context, in return client.s.FindAllShardsInKeyspace(ctx, in) } +// ForceCutOverSchemaMigration is part of the vtctlservicepb.VtctldClient interface. +func (client *localVtctldClient) ForceCutOverSchemaMigration(ctx context.Context, in *vtctldatapb.ForceCutOverSchemaMigrationRequest, opts ...grpc.CallOption) (*vtctldatapb.ForceCutOverSchemaMigrationResponse, error) { + return client.s.ForceCutOverSchemaMigration(ctx, in) +} + // GetBackups is part of the vtctlservicepb.VtctldClient interface. func (client *localVtctldClient) GetBackups(ctx context.Context, in *vtctldatapb.GetBackupsRequest, opts ...grpc.CallOption) (*vtctldatapb.GetBackupsResponse, error) { return client.s.GetBackups(ctx, in) From a3514a52bd71b7e595e64f1a0b1f7dbd19c1f0ce Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 18:02:49 -0500 Subject: [PATCH 03/10] Add positive value checks Signed-off-by: Matt Lord --- .../vtctldclient/command/vreplication/vdiff/vdiff.go | 10 ++++++++++ go/test/endtoend/vreplication/vdiff2_test.go | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index f512242510f..0d1dd4c1af3 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -113,6 +113,16 @@ var ( createOptions.Tables[i] = strings.TrimSpace(table) } } + // Enforce positive values for limits and max values. + if createOptions.Limit < 1 { + return fmt.Errorf("--limit must be a positive value greater than 1") + } + if createOptions.MaxReportSampleRows < 0 { + return fmt.Errorf("--max-report-sample-rows must be a positive value") + } + if createOptions.MaxExtraRowsToCompare < 0 { + return fmt.Errorf("--max-extra-rows-to-compare must be a positive value") + } return nil } diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index a6c75da1b19..0948b311d6a 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -261,8 +261,8 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell "timeout_seconds": "60", } pickerOpts := map[string]string{ - "source_cell": "zone1,zone2,zone3,zonefoo", - "target_cell": "zone1,zone2,zone3,zonefoo", + "source_cell": "zone1,zone2,zone3,zonefoosource", + "target_cell": "zone1,zone2,zone3,zonefootarget", "tablet_types": "replica,primary,rdonly", } reportOpts := map[string]string{ From afb1fe1b2589299d7af02a1d76158bdd079386ee Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 18:23:26 -0500 Subject: [PATCH 04/10] Address review comments Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index 0d1dd4c1af3..7279638f199 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -118,10 +118,10 @@ var ( return fmt.Errorf("--limit must be a positive value greater than 1") } if createOptions.MaxReportSampleRows < 0 { - return fmt.Errorf("--max-report-sample-rows must be a positive value") + return fmt.Errorf("--max-report-sample-rows must not be a negative value") } if createOptions.MaxExtraRowsToCompare < 0 { - return fmt.Errorf("--max-extra-rows-to-compare must be a positive value") + return fmt.Errorf("--max-extra-rows-to-compare must not be a negative value") } return nil } From 602825ced833fc86c2154d2d47428522adbff8b5 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 18:24:15 -0500 Subject: [PATCH 05/10] Nitty comment Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index 7279638f199..9d966e6d001 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -113,7 +113,7 @@ var ( createOptions.Tables[i] = strings.TrimSpace(table) } } - // Enforce positive values for limits and max values. + // Enforce non-negative values for limits and max values. if createOptions.Limit < 1 { return fmt.Errorf("--limit must be a positive value greater than 1") } From 70a0fecbb0125ef0c5ac98032421fe7fc50f5dbd Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 18:27:24 -0500 Subject: [PATCH 06/10] Can't stop nitting Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index 9d966e6d001..2b86045e929 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -115,7 +115,7 @@ var ( } // Enforce non-negative values for limits and max values. if createOptions.Limit < 1 { - return fmt.Errorf("--limit must be a positive value greater than 1") + return fmt.Errorf("--limit must be a positive value of at least 1") } if createOptions.MaxReportSampleRows < 0 { return fmt.Errorf("--max-report-sample-rows must not be a negative value") From 9339abda9dcd319d9efe9b6151ff1efd3108a42c Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 19:10:57 -0500 Subject: [PATCH 07/10] Minor changes after self review Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/vdiff2_test.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index 0948b311d6a..2d39386bf7e 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -46,7 +46,7 @@ type testCase struct { stop bool // test stop functionality with this workflow testCLIErrors bool // test CLI errors against this workflow (only needs to be done once) testCLICreateWait bool // test CLI create and wait until done against this workflow (only needs to be done once) - testCLIFlagHandling bool + testCLIFlagHandling bool // test vtctldclient flag handling from end-to-end } const ( @@ -73,7 +73,7 @@ var testCases = []*testCase{ resumeInsert: `insert into customer(cid, name, typ) values(1992234, 'Testy McTester (redux)', 'enterprise')`, testCLIErrors: true, // test for errors in the simplest workflow testCLICreateWait: true, // test wait on create feature against simplest workflow - testCLIFlagHandling: true, + testCLIFlagHandling: true, // test flag handling end-to-end against simplest workflow }, { name: "Reshard Merge/split 2 to 3", @@ -281,19 +281,20 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell "--tablet-types-in-preference-order=false", "--format=json") require.NoError(t, err, "vdiff command failed: %s", res) jsonRes := gjson.Parse(res) - vdid := jsonRes.Get("UUID").String() - _, err = uuid.Parse(vdid) - require.NoError(t, err, "invalid UUID: %s", vdid) + vduuid, err := uuid.Parse(jsonRes.Get("UUID").String()) + require.NoError(t, err, "invalid UUID: %s", jsonRes.Get("UUID").String()) // Confirm that the options were set and saved correctly. query := sqlparser.BuildParsedQuery("select options from %s.vdiff where vdiff_uuid = %s", - sidecarDBIdentifier, encodeString(vdid)).Query + sidecarDBIdentifier, encodeString(vduuid.String())).Query tablets := vc.getVttabletsInKeyspace(t, cell, targetKs, "PRIMARY") require.Greater(t, len(tablets), 0, "no primary tablets found in keyspace %s", targetKs) tablet := maps.Values(tablets)[0] qres, err := tablet.QueryTablet(query, targetKs, false) - require.NoError(t, err, "query failed: %s", query) - require.NotNil(t, qres, "query returned nil result: %s", query) + require.NoError(t, err, "query %q failed: %v", query, err) + require.NotNil(t, qres, "query %q returned nil result", query) // Should never happen + require.Equal(t, 1, len(qres.Rows), "query %q returned %d rows, expected 1", query, len(qres.Rows)) + require.Equal(t, 1, len(qres.Rows[0]), "query %q returned %d columns, expected 1", query, len(qres.Rows[0])) jsonRes = gjson.Parse(qres.Rows[0][0].ToString()) for key, val := range coreOpts { require.Equal(t, val, jsonRes.Get("core_options."+key).String(), "unexpected value for key core_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("core_options."+key).String()) From f443b414daea01ac3f6d62b25ea1bca1dc981796 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 19:44:13 -0500 Subject: [PATCH 08/10] Use protos directly in e2e test Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/vdiff2_test.go | 71 +++++++++++--------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index 2d39386bf7e..58a985baf6d 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -26,8 +26,11 @@ import ( "github.com/stretchr/testify/require" "github.com/tidwall/gjson" "golang.org/x/exp/maps" + "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/proto" "vitess.io/vitess/go/test/endtoend/cluster" + "vitess.io/vitess/go/vt/proto/tabletmanagerdata" "vitess.io/vitess/go/vt/sqlparser" ) @@ -253,32 +256,40 @@ func testCLIErrors(t *testing.T, ksWorkflow, cells string) { // from vtctldclient->vtctld->vttablet->mysqld. func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell) { // Keys are in the tabletmanagerdata.VDiff*Options proto message definitions. - coreOpts := map[string]string{ - "max_rows": "999", - "max_extra_rows_to_compare": "777", - "auto_retry": "true", - "update_table_stats": "true", - "timeout_seconds": "60", - } - pickerOpts := map[string]string{ - "source_cell": "zone1,zone2,zone3,zonefoosource", - "target_cell": "zone1,zone2,zone3,zonefootarget", - "tablet_types": "replica,primary,rdonly", - } - reportOpts := map[string]string{ - "max_sample_rows": "888", - "only_pks": "true", + options := &tabletmanagerdata.VDiffOptions{ + CoreOptions: &tabletmanagerdata.VDiffCoreOptions{ + MaxRows: 999, + MaxExtraRowsToCompare: 777, + AutoRetry: true, + UpdateTableStats: true, + TimeoutSeconds: 60, + }, + PickerOptions: &tabletmanagerdata.VDiffPickerOptions{ + SourceCell: "zone1,zone2,zone3,zonefoosource", + TargetCell: "zone1,zone2,zone3,zonefootarget", + TabletTypes: "replica,primary,rdonly", + }, + ReportOptions: &tabletmanagerdata.VDiffReportOptions{ + MaxSampleRows: 888, + OnlyPks: true, + }, } t.Run("Client flag handling", func(t *testing.T) { res, err := vc.VtctldClient.ExecuteCommandWithOutput("vdiff", "--target-keyspace", targetKs, "--workflow", workflowName, - "create", "--limit", coreOpts["max_rows"], "--max-report-sample-rows", reportOpts["max_sample_rows"], - "--max-extra-rows-to-compare", coreOpts["max_extra_rows_to_compare"], "--filtered-replication-wait-time", - coreOpts["timeout_seconds"]+"s", "--source-cells", pickerOpts["source_cell"], "--target-cells", - pickerOpts["target_cell"], "--tablet-types", pickerOpts["tablet_types"], - fmt.Sprintf("--update-table-stats=%s", coreOpts["update_table_stats"]), - fmt.Sprintf("--auto-retry=%s", coreOpts["auto_retry"]), fmt.Sprintf("--only-pks=%s", reportOpts["only_pks"]), - "--tablet-types-in-preference-order=false", "--format=json") + "create", + "--limit", fmt.Sprintf("%d", options.CoreOptions.MaxRows), + "--max-report-sample-rows", fmt.Sprintf("%d", options.ReportOptions.MaxSampleRows), + "--max-extra-rows-to-compare", fmt.Sprintf("%d", options.CoreOptions.MaxExtraRowsToCompare), + "--filtered-replication-wait-time", fmt.Sprintf("%v", time.Duration(options.CoreOptions.TimeoutSeconds)*time.Second), + "--source-cells", options.PickerOptions.SourceCell, + "--target-cells", options.PickerOptions.TargetCell, + "--tablet-types", options.PickerOptions.TabletTypes, + fmt.Sprintf("--update-table-stats=%t", options.CoreOptions.UpdateTableStats), + fmt.Sprintf("--auto-retry=%t", options.CoreOptions.AutoRetry), + fmt.Sprintf("--only-pks=%t", options.ReportOptions.OnlyPks), + "--tablet-types-in-preference-order=false", // So tablet_types should not start with "in_order:", which is the default + "--format=json") // So we can easily grab the UUID require.NoError(t, err, "vdiff command failed: %s", res) jsonRes := gjson.Parse(res) vduuid, err := uuid.Parse(jsonRes.Get("UUID").String()) @@ -295,16 +306,12 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell require.NotNil(t, qres, "query %q returned nil result", query) // Should never happen require.Equal(t, 1, len(qres.Rows), "query %q returned %d rows, expected 1", query, len(qres.Rows)) require.Equal(t, 1, len(qres.Rows[0]), "query %q returned %d columns, expected 1", query, len(qres.Rows[0])) - jsonRes = gjson.Parse(qres.Rows[0][0].ToString()) - for key, val := range coreOpts { - require.Equal(t, val, jsonRes.Get("core_options."+key).String(), "unexpected value for key core_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("core_options."+key).String()) - } - for key, val := range pickerOpts { - require.Equal(t, val, jsonRes.Get("picker_options."+key).String(), "unexpected value for key picker_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("picker_options."+key).String()) - } - for key, val := range reportOpts { - require.Equal(t, val, jsonRes.Get("report_options."+key).String(), "unexpected value for key report_options.%s; expected: %s, got: %s", key, val, jsonRes.Get("report_options."+key).String()) - } + storedOptions := &tabletmanagerdata.VDiffOptions{} + bytes, err := qres.Rows[0][0].ToBytes() + require.NoError(t, err, "failed to convert result %+v to bytes: %v", qres.Rows[0], err) + err = protojson.Unmarshal(bytes, storedOptions) + require.NoError(t, err, "failed to unmarshal result %s to a %T: %v", string(bytes), storedOptions, err) + require.True(t, proto.Equal(options, storedOptions), "stored options %v != expected options %v", storedOptions, options) }) } From 515ce0a2fed485f2e3300bc3a05313ce9612c81a Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 19:53:45 -0500 Subject: [PATCH 09/10] Last set of self-review nits (I swear) Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/vdiff2_test.go | 38 ++++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index 58a985baf6d..b4753750871 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -30,8 +30,9 @@ import ( "google.golang.org/protobuf/proto" "vitess.io/vitess/go/test/endtoend/cluster" - "vitess.io/vitess/go/vt/proto/tabletmanagerdata" "vitess.io/vitess/go/vt/sqlparser" + + tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata" ) type testCase struct { @@ -255,21 +256,20 @@ func testCLIErrors(t *testing.T, ksWorkflow, cells string) { // testCLIFlagHandling tests that the vtctldclient CLI flags are handled correctly // from vtctldclient->vtctld->vttablet->mysqld. func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell) { - // Keys are in the tabletmanagerdata.VDiff*Options proto message definitions. - options := &tabletmanagerdata.VDiffOptions{ - CoreOptions: &tabletmanagerdata.VDiffCoreOptions{ + expectedOptions := &tabletmanagerdatapb.VDiffOptions{ + CoreOptions: &tabletmanagerdatapb.VDiffCoreOptions{ MaxRows: 999, MaxExtraRowsToCompare: 777, AutoRetry: true, UpdateTableStats: true, TimeoutSeconds: 60, }, - PickerOptions: &tabletmanagerdata.VDiffPickerOptions{ + PickerOptions: &tabletmanagerdatapb.VDiffPickerOptions{ SourceCell: "zone1,zone2,zone3,zonefoosource", TargetCell: "zone1,zone2,zone3,zonefootarget", TabletTypes: "replica,primary,rdonly", }, - ReportOptions: &tabletmanagerdata.VDiffReportOptions{ + ReportOptions: &tabletmanagerdatapb.VDiffReportOptions{ MaxSampleRows: 888, OnlyPks: true, }, @@ -278,16 +278,16 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell t.Run("Client flag handling", func(t *testing.T) { res, err := vc.VtctldClient.ExecuteCommandWithOutput("vdiff", "--target-keyspace", targetKs, "--workflow", workflowName, "create", - "--limit", fmt.Sprintf("%d", options.CoreOptions.MaxRows), - "--max-report-sample-rows", fmt.Sprintf("%d", options.ReportOptions.MaxSampleRows), - "--max-extra-rows-to-compare", fmt.Sprintf("%d", options.CoreOptions.MaxExtraRowsToCompare), - "--filtered-replication-wait-time", fmt.Sprintf("%v", time.Duration(options.CoreOptions.TimeoutSeconds)*time.Second), - "--source-cells", options.PickerOptions.SourceCell, - "--target-cells", options.PickerOptions.TargetCell, - "--tablet-types", options.PickerOptions.TabletTypes, - fmt.Sprintf("--update-table-stats=%t", options.CoreOptions.UpdateTableStats), - fmt.Sprintf("--auto-retry=%t", options.CoreOptions.AutoRetry), - fmt.Sprintf("--only-pks=%t", options.ReportOptions.OnlyPks), + "--limit", fmt.Sprintf("%d", expectedOptions.CoreOptions.MaxRows), + "--max-report-sample-rows", fmt.Sprintf("%d", expectedOptions.ReportOptions.MaxSampleRows), + "--max-extra-rows-to-compare", fmt.Sprintf("%d", expectedOptions.CoreOptions.MaxExtraRowsToCompare), + "--filtered-replication-wait-time", fmt.Sprintf("%v", time.Duration(expectedOptions.CoreOptions.TimeoutSeconds)*time.Second), + "--source-cells", expectedOptions.PickerOptions.SourceCell, + "--target-cells", expectedOptions.PickerOptions.TargetCell, + "--tablet-types", expectedOptions.PickerOptions.TabletTypes, + fmt.Sprintf("--update-table-stats=%t", expectedOptions.CoreOptions.UpdateTableStats), + fmt.Sprintf("--auto-retry=%t", expectedOptions.CoreOptions.AutoRetry), + fmt.Sprintf("--only-pks=%t", expectedOptions.ReportOptions.OnlyPks), "--tablet-types-in-preference-order=false", // So tablet_types should not start with "in_order:", which is the default "--format=json") // So we can easily grab the UUID require.NoError(t, err, "vdiff command failed: %s", res) @@ -295,7 +295,7 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell vduuid, err := uuid.Parse(jsonRes.Get("UUID").String()) require.NoError(t, err, "invalid UUID: %s", jsonRes.Get("UUID").String()) - // Confirm that the options were set and saved correctly. + // Confirm that the options were passed through and saved correctly. query := sqlparser.BuildParsedQuery("select options from %s.vdiff where vdiff_uuid = %s", sidecarDBIdentifier, encodeString(vduuid.String())).Query tablets := vc.getVttabletsInKeyspace(t, cell, targetKs, "PRIMARY") @@ -306,12 +306,12 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell require.NotNil(t, qres, "query %q returned nil result", query) // Should never happen require.Equal(t, 1, len(qres.Rows), "query %q returned %d rows, expected 1", query, len(qres.Rows)) require.Equal(t, 1, len(qres.Rows[0]), "query %q returned %d columns, expected 1", query, len(qres.Rows[0])) - storedOptions := &tabletmanagerdata.VDiffOptions{} + storedOptions := &tabletmanagerdatapb.VDiffOptions{} bytes, err := qres.Rows[0][0].ToBytes() require.NoError(t, err, "failed to convert result %+v to bytes: %v", qres.Rows[0], err) err = protojson.Unmarshal(bytes, storedOptions) require.NoError(t, err, "failed to unmarshal result %s to a %T: %v", string(bytes), storedOptions, err) - require.True(t, proto.Equal(options, storedOptions), "stored options %v != expected options %v", storedOptions, options) + require.True(t, proto.Equal(expectedOptions, storedOptions), "stored options %v != expected options %v", storedOptions, expectedOptions) }) } From 90507a7fa473d9227bca4d7af27faf2059e2c343 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Wed, 13 Dec 2023 20:12:35 -0500 Subject: [PATCH 10/10] Halp... me... Signed-off-by: Matt Lord --- go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go index 2b86045e929..722578fe411 100644 --- a/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go +++ b/go/cmd/vtctldclient/command/vreplication/vdiff/vdiff.go @@ -113,9 +113,9 @@ var ( createOptions.Tables[i] = strings.TrimSpace(table) } } - // Enforce non-negative values for limits and max values. + // Enforce non-negative values for limits and max options. if createOptions.Limit < 1 { - return fmt.Errorf("--limit must be a positive value of at least 1") + return fmt.Errorf("--limit must be a positive value") } if createOptions.MaxReportSampleRows < 0 { return fmt.Errorf("--max-report-sample-rows must not be a negative value")