LookupVindex: Implement `internalize` command for lookup vindexes #17429

beingnoble03 · 2024-12-24T19:50:57Z

Description

This PR

Implements the internalize and complete command for lookup vindexes
Also, modifies the externalize command where we stop the workflow and mark it as frozen instead of deleting it.
complete command checks if the lookup vindex is externalized, and if it has an owner, it deletes the workflow.
internalize command starts the frozen streams (if there's an owner) and sets the write_only vindex param back to true (which was deleted when lookup vindex was externalized.)

Related Issue(s)

Fixes Feature Request: LookupVindex has no internalize command (equivalent to ReverseTraffic) #15985

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

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

vitess-bot · 2024-12-24T19:50:59Z

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

codecov · 2024-12-29T20:28:33Z

Codecov Report

Attention: Patch coverage is 53.20755% with 124 lines in your changes missing coverage. Please review.

Project coverage is 67.67%. Comparing base (9383943) to head (18b7de1).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
.../command/vreplication/lookupvindex/lookupvindex.go	12.50%	56 Missing ⚠️
go/vt/vtctl/grpcvtctldserver/server.go	0.00%	24 Missing ⚠️
go/vt/vtctl/workflow/server.go	82.30%	20 Missing ⚠️
go/vt/vtctl/grpcvtctldclient/client_gen.go	0.00%	10 Missing ⚠️
go/vt/vtctl/workflow/lookup_vindex.go	76.47%	8 Missing ⚠️
go/vt/vtctl/localvtctldclient/client_gen.go	0.00%	4 Missing ⚠️
go/vt/vtctl/workflow/utils.go	81.81%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #17429    +/-   ##
========================================
  Coverage   67.67%   67.67%            
========================================
  Files        1583     1584     +1     
  Lines      254363   254703   +340     
========================================
+ Hits       172140   172376   +236     
- Misses      82223    82327   +104

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

go/vt/vtctl/workflow/server.go

go/test/endtoend/vreplication/lookup_vindex_test.go

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

mattlord

This is looking good! I only had some minor comments and suggestions. Let me know what you think.

mattlord · 2025-01-06T19:21:04Z

go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go

@@ -158,7 +178,7 @@ var (
 	// externalize makes a LookupVindexExternalize call to a vtctld.
 	externalize = &cobra.Command{
 		Use:                   "externalize",
-		Short:                 "Externalize the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be deleted.",
+		Short:                 "Externalize the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be stopped.",


Would it be challenging or problematic to add a --delete flag for externalize for those that want to retain the old behavior? Then we can continue using the WorkflowDeleted response field too.

mattlord · 2025-01-06T19:29:12Z

go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go

+	if resp.WorkflowDeleted {
+		output = output + fmt.Sprintf(" and the %s VReplication workflow has been deleted", baseOptions.Name)
+	}


I think it would also be nice to note when it was NOT deleted and why as the user is likely going to want to follow-up on this at some point.

Nit, but I don't think we need to specify the name yet again here, we can instead say and the VReplication workflow...

mattlord · 2025-01-06T19:31:21Z

go/cmd/vtctldclient/command/vreplication/lookupvindex/lookupvindex.go

+	if resp.WorkflowStarted {
+		output = output + fmt.Sprintf(" and the %s VReplication workflow has been started", baseOptions.Name)


Same comments here from the complete command.

mattlord · 2025-01-06T21:30:06Z

go/vt/vtctl/workflow/server.go

@@ -127,6 +127,9 @@ const (
 	lockTablesCycles = 2
 	// Time to wait between LOCK TABLES cycles on the sources during SwitchWrites.
 	lockTablesCycleDelay = time.Duration(100 * time.Millisecond)
+
+	SqlFreezeWorkflow   = "update _vt.vreplication set message = '%s' where db_name=%s and workflow=%s"


We can set the message directly here using the const as we always want to freeze it with the query.

mattlord · 2025-01-06T21:33:51Z

go/vt/vtctl/workflow/server.go

+// LookupVindexComplete checks if the lookup vindex has been externalized,
+// and if the vindex has an owner, it deletes the workflow.


I think that the criteria should be the lookup vindex backfill workflow is "done", meaning it was externalized, which in turn means that it's no longer write_only and it has an owner (the source table).

Same comment about the validateExternalized()

mattlord · 2025-01-06T21:38:01Z

go/vt/vtctl/workflow/server.go

+			// Stop the workflow.
+			_, err = s.tmc.UpdateVReplicationWorkflow(ctx, tabletInfo.Tablet, &tabletmanagerdatapb.UpdateVReplicationWorkflowRequest{
+				Workflow: req.Name,
+				State:    ptr.Of(binlogdatapb.VReplicationWorkflowState_Stopped),
+			})
+			if err != nil {
+				return vterrors.Wrapf(err, "failed to stop workflow %s on shard %s/%s", req.Name, tabletInfo.Keyspace, tabletInfo.Shard)
+			}
+			// Mark workflow as frozen.
+			query := fmt.Sprintf(SqlFreezeWorkflow, Frozen,
+				encodeString(tabletInfo.DbName()), encodeString(req.Name))
+			_, err = s.tmc.VReplicationExec(ctx, tabletInfo.Tablet, query)


I think that we should update the message in the UpdateVReplicationWorkflow call above, which means we'll have to add support for that to the RPC as an optional proto field.

Done, thanks for this! This is much cleaner now.

mattlord · 2025-01-06T21:40:01Z

go/vt/vtctl/workflow/server.go

+// LookupVindexInternalize internalizes a lookup vindex. If the vindex has an
+// owner then the stopped workflow will also be started.


I think it's also worth checking that it does not have the write_only flag set as well.

I think it's worth defining what it means for a vindex to be externalized as 1) having an owner 2) having write_only == false, so that we can check for that everywhere using a function.

Added check for write_only, but shouldn't we check if all the streams are running if it's an unowned vindex, and all the streams are stopped if it's an owned vindex? Added validateExternalized() func for this, please have a look.

Please let me know if that's the right thing?

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

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

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

LookupVindex: Implement internalize command for lookup vindexes

3ab44c3

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

github-actions bot added this to the v22.0.0 milestone Dec 24, 2024

beingnoble03 added 3 commits December 26, 2024 19:51

LookupVindex: Implement LookupVindexComplete

944c39c

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

Merge branch 'main' into lookup-vindex-internalize

7038c78

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

test(LookupVindex): Add tests for internalize and complete

128198d

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

beingnoble03 marked this pull request as ready for review December 30, 2024 06:30

beingnoble03 requested review from deepthi, harshit-gangal, mattlord, ajm188, notfelineit, rohit-nayak-ps, GuptaManan100 and shlomi-noach as code owners December 30, 2024 06:30

rohit-nayak-ps reviewed Jan 3, 2025

View reviewed changes

go/vt/vtctl/workflow/server.go Outdated Show resolved Hide resolved

go/test/endtoend/vreplication/lookup_vindex_test.go Show resolved Hide resolved

Add more e2e tests and helper func to get vindex

983a415

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

rohit-nayak-ps approved these changes Jan 5, 2025

View reviewed changes

mattlord reviewed Jan 6, 2025

View reviewed changes

Add delete flag for externalize command to continue old behaviour

ee296d2

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

beingnoble03 force-pushed the lookup-vindex-internalize branch from 54753a4 to ee296d2 Compare January 8, 2025 21:51

beingnoble03 added 4 commits January 9, 2025 03:42

test: Fix failing TestInternalizeLookupVindex sub-tests

f11a0a7

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

Extend support for stream message in UpdateVReplicationWorkflow

f6f42ba

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

test: Add/Fix unit tests for updating message with UpdateVReplication…

f63d9f3

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

test: Add unit test when we want workflow deleted after externalizing

18b7de1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LookupVindex: Implement `internalize` command for lookup vindexes #17429

LookupVindex: Implement `internalize` command for lookup vindexes #17429

beingnoble03 commented Dec 24, 2024 •

edited

Loading

vitess-bot bot commented Dec 24, 2024

codecov bot commented Dec 29, 2024 •

edited

Loading

mattlord left a comment

mattlord Jan 6, 2025

beingnoble03 Jan 8, 2025

mattlord Jan 6, 2025

beingnoble03 Jan 8, 2025

mattlord Jan 6, 2025

beingnoble03 Jan 8, 2025

mattlord Jan 6, 2025

beingnoble03 Jan 8, 2025

mattlord Jan 6, 2025

beingnoble03 Jan 8, 2025

mattlord Jan 6, 2025

beingnoble03 Jan 9, 2025

mattlord Jan 6, 2025

beingnoble03 Jan 8, 2025

		if resp.WorkflowStarted {
		output = output + fmt.Sprintf(" and the %s VReplication workflow has been started", baseOptions.Name)

		// LookupVindexComplete checks if the lookup vindex has been externalized,
		// and if the vindex has an owner, it deletes the workflow.

		// LookupVindexInternalize internalizes a lookup vindex. If the vindex has an
		// owner then the stopped workflow will also be started.

LookupVindex: Implement internalize command for lookup vindexes #17429

Are you sure you want to change the base?

LookupVindex: Implement internalize command for lookup vindexes #17429

Conversation

beingnoble03 commented Dec 24, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Dec 24, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Dec 29, 2024 • edited Loading

Codecov Report

mattlord 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

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

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

LookupVindex: Implement `internalize` command for lookup vindexes #17429

LookupVindex: Implement `internalize` command for lookup vindexes #17429

beingnoble03 commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 29, 2024 •

edited

Loading