fix: Avoid creation of workflows with non-empty tables in target keyspace #16874

beingnoble03 · 2024-10-02T10:21:12Z

Description

This PR adds a validation check for empty tables in the target keyspace while creating workflows, in deploySchema(). Addresses all the comments in #16826.

Related Issue(s)

MoveTables Create: ensure that none of the tables to be moved have data #16817

Screenshots

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2024-10-02T10:21:14Z

…pace Signed-off-by: Noble Mittal <[email protected]>

Signed-off-by: Noble Mittal <[email protected]>

codecov · 2024-10-04T08:50:56Z

Codecov Report

Attention: Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 69.36%. Comparing base (7eaf236) to head (492406e).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtctl/workflow/materializer.go	83.33%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16874      +/-   ##
==========================================
+ Coverage   69.34%   69.36%   +0.02%     
==========================================
  Files        1571     1571              
  Lines      204179   204221      +42     
==========================================
+ Hits       141586   141668      +82     
+ Misses      62593    62553      -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rohit-nayak-ps

Looking good! Can you address the comments and also update the description with more detail on how you have implemented the PR.

rohit-nayak-ps · 2024-10-07T11:43:43Z

go/vt/vttablet/tabletmanager/framework_test.go

@@ -480,8 +480,8 @@ func (tmc *fakeTMClient) VReplicationExec(ctx context.Context, tablet *topodatap
 	}
 	for qry, res := range tmc.vreQueries[int(tablet.Alias.Uid)] {


Why did you need to make this change from regexp.MustCompile(qry) to regexp.MustCompile(qry[1:]) ?
It is not correct afaik: this is supposed to allow regexp queries, so the / at the beginning should be required.

Your second fix for matching query instead of qry is a good catch.

We need to remove the / at the beginning, for regexp in go, we don't need that for regexp matching. This was probably a mistake. We already have similar logic in our codebase: e.g. https://github.com/vitessio/vitess/blob/main/go/vt/vtctl/workflow/framework_test.go#L450-L463

go/vt/vtctl/workflow/utils.go

Signed-off-by: Noble Mittal <[email protected]>

rohit-nayak-ps · 2024-10-10T11:17:42Z

go/vt/vtctl/workflow/utils.go

+// containing a list of non-empty tables.
+func validateEmptyTables(ctx context.Context, ts *topo.Server, tmc tmclient.TabletManagerClient, shards []*topo.ShardInfo, tableSettings []*vtctldatapb.TableMaterializeSettings) error {
+	var mu sync.Mutex
+	isTableFaulty := map[string]bool{}


Change terminology from "Faulty" to "NonEmpty" like hasTableData or isNonEmpty. Similarly faultyTables => nonEmptyTables or tablesWithData ...

Signed-off-by: Noble Mittal <[email protected]>

mattlord

This is great, @beingnoble03 ! I only had minor nits and suggestions, can you huddle up with @rohit-nayak-ps and decide what changes you'd like to make from my comments? They are not correctness issues so I will leave it to the two of you to decide.

This is the only one that I really think we should probably do: #16874 (comment)

Thanks! ❤️

mattlord · 2024-10-11T02:13:26Z

go/vt/vtctl/workflow/materializer.go

@@ -290,6 +290,15 @@ func (mz *materializer) deploySchema() error {
 		}
 	}

+	// Check if any table being moved is already non-empty in the target keyspace.
+	// Skip this check for multi-tenant migrations.
+	if !mz.IsMultiTenantMigration() {


A nice future addition would be to check for any existing tenant data in each table on the target too.

mattlord · 2024-10-11T02:14:41Z

go/vt/vtctl/workflow/materializer.go

+	// Check if any table being moved is already non-empty in the target keyspace.
+	// Skip this check for multi-tenant migrations.
+	if !mz.IsMultiTenantMigration() {
+		err := validateEmptyTables(mz.ctx, mz.ts, mz.tmc, mz.targetShards, mz.ms.TableSettings)


We're passing in a lot of mz members, which tells me it's probably better to have mz as the method receiver, no?

mz.validateEmptyTables()

I thought about this too, but went with the other approach anyway. Refactored the function with the receiver, it looks much cleaner now!
Also, if the method receiver is materializer, it makes sense to have validateEmptyTables in materializer.go. So, moved it from utils.go.

go/vt/vtctl/workflow/utils.go

go/vt/vtctl/workflow/materializer.go

go/vt/vtctl/workflow/utils.go

mattlord · 2024-10-11T02:30:15Z

go/vt/vtctl/workflow/utils.go

+		eg, groupCtx := errgroup.WithContext(ctx)
+		eg.SetLimit(20)
+
+		for _, ts := range tableSettings {


It might be worth adding the shard and table to the errors in this loop as there could be 1,000 shards and you would want to know what specific shard had a table with data in it, and e.g. what shard and table we encountered an error in processing.

can we do this in a follow-up PR?

go/vt/vtctl/workflow/utils.go

Signed-off-by: Noble Mittal <[email protected]>

github-actions bot added this to the v21.0.0 milestone Oct 2, 2024

fix: Avoid creation of workflows with non-empty tables in target keys…

e1bff4b

…pace Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 force-pushed the workflow-duplicate-tables branch from 45e3cba to 305bfd7 Compare October 2, 2024 15:25

beingnoble03 added 3 commits October 4, 2024 12:51

test: Add/Fix unit tests for the check

d0c2b5c

Signed-off-by: Noble Mittal <[email protected]>

Merge branch 'main' into workflow-duplicate-tables

6c454ac

Signed-off-by: Noble Mittal <[email protected]>

fix: Add mock query in TestCreateLookupVindexFailures

1f30481

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 force-pushed the workflow-duplicate-tables branch from 305bfd7 to 1f30481 Compare October 4, 2024 08:30

beingnoble03 marked this pull request as ready for review October 4, 2024 08:58

beingnoble03 requested review from deepthi, GuptaManan100, rohit-nayak-ps, shlomi-noach and ajm188 as code owners October 4, 2024 08:58

rohit-nayak-ps reviewed Oct 7, 2024

View reviewed changes

vitess-bot modified the milestones: v21.0.0, v22.0.0 Oct 8, 2024

beingnoble03 added 2 commits October 8, 2024 23:47

refac: Use errgroup to run table queries in parallel

6561442

Signed-off-by: Noble Mittal <[email protected]>

Merge main into workflow-duplicate-tables

bd4d559

Signed-off-by: Noble Mittal <[email protected]>

test: Fix failing TestShardedAutoIncHandling by mocking query

483841d

Signed-off-by: Noble Mittal <[email protected]>

rohit-nayak-ps approved these changes Oct 10, 2024

View reviewed changes

refac: Rename faultyTables to nonEmptyTables

786bd7c

Signed-off-by: Noble Mittal <[email protected]>

mattlord approved these changes Oct 11, 2024

View reviewed changes

beingnoble03 added 3 commits October 16, 2024 01:59

Address review comments

92412cc

Signed-off-by: Noble Mittal <[email protected]>

Merge branch 'main' into workflow-duplicate-tables

10b5a45

Signed-off-by: Noble Mittal <[email protected]>

Trigger workflows

492406e

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 mentioned this pull request Oct 16, 2024

fix: Avoid creating MoveTables in case of non-empty target tables #16826

Closed

5 tasks

rohit-nayak-ps merged commit b8ba593 into vitessio:main Oct 18, 2024
98 checks passed

rohit-nayak-ps mentioned this pull request Oct 19, 2024

MoveTables Create: ensure that none of the tables to be moved have data #16817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Avoid creation of workflows with non-empty tables in target keyspace #16874

fix: Avoid creation of workflows with non-empty tables in target keyspace #16874

beingnoble03 commented Oct 2, 2024 •

edited

Loading

vitess-bot bot commented Oct 2, 2024

codecov bot commented Oct 4, 2024 •

edited

Loading

rohit-nayak-ps left a comment

rohit-nayak-ps Oct 7, 2024

beingnoble03 Oct 8, 2024

rohit-nayak-ps Oct 10, 2024

beingnoble03 Oct 10, 2024

mattlord left a comment •

edited

Loading

mattlord Oct 11, 2024

mattlord Oct 11, 2024

beingnoble03 Oct 15, 2024

mattlord Oct 11, 2024

beingnoble03 Oct 15, 2024

		@@ -480,8 +480,8 @@ func (tmc fakeTMClient) VReplicationExec(ctx context.Context, tablet topodatap
		}
		for qry, res := range tmc.vreQueries[int(tablet.Alias.Uid)] {

fix: Avoid creation of workflows with non-empty tables in target keyspace #16874

fix: Avoid creation of workflows with non-empty tables in target keyspace #16874

Conversation

beingnoble03 commented Oct 2, 2024 • edited Loading

Description

Related Issue(s)

Screenshots

Checklist

Deployment Notes

vitess-bot bot commented Oct 2, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlord left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beingnoble03 commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

mattlord left a comment •

edited

Loading