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 52.59259% with 128 lines in your changes missing coverage. Please review.

Project coverage is 67.68%. Comparing base (9383943) to head (1631cd2).
Report is 32 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	79.13%	24 Missing ⚠️
go/vt/vtctl/grpcvtctldclient/client_gen.go	0.00%	10 Missing ⚠️
go/vt/vtctl/workflow/lookup_vindex.go	78.37%	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.68%   +0.01%     
==========================================
  Files        1583     1584       +1     
  Lines      254363   254753     +390     
==========================================
+ Hits       172140   172440     +300     
- Misses      82223    82313      +90

☔ 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.

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

go/vt/vtctl/workflow/server.go

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

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

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

mattlord

This is looking really good! I still had a few outstanding questions/concerns though. Happy to sit down and walk through things if you like. I could be missing or misunderstanding some things as well so we could quickly get on the same page.

go/vt/vttablet/tabletmanager/rpc_vreplication.go

go/vt/vtctl/workflow/lookup_vindex.go

go/vt/vtctl/workflow/server.go

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

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

mattlord

The code all looks great now! The only thing left, IMO, is updating some of the user facing text (and dev facing comments) based on the current behavior after the changes. As you now know, this can easily get confusing so any clarity we can add is worth it.

Hopefully it makes sense. Feel free to grab me on slack if not. Thank you for hanging in there! 🙂

mattlord · 2025-01-17T18:47:21Z

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

+	// complete makes a LookupVindexComplete call to a vtctld.
+	complete = &cobra.Command{
+		Use:                   "complete",
+		Short:                 "Complete the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be deleted.",


This should be tweaked a bit now, I think. No? Maybe:

"Complete the LookupVindex workflow. The VIndex must have been previously externalized. If you want to delete the workflow without externalizing the VIndex then use the cancel command instead."

mattlord · 2025-01-17T18:53:00Z

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

+	// internalize makes a LookupVindexInternalize call to a vtctld.
+	internalize = &cobra.Command{
+		Use:                   "internalize",
+		Short:                 "Internalize the Lookup Vindex. If the Vindex has an owner the VReplication workflow will also be started.",


IMO we're being a little ambiguous about the LookupVindex workflow and the VIndex that it backfills. Also, isn't it that we will restart the workflow if it does NOT have an owner? Doesn't it HAVE to not have an owner for it to be considered internalized? Are we referring to the state of the VIndex before we change anything? Internalize should require that externalize had been previously done, which means that there must be an owner (the backing table). IMO this is clearer:

"Internalize the VIndex again to continue the back fill, making it unusable for queries again."

mattlord · 2025-01-17T18:53:42Z

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

+	if resp.WorkflowDeleted {
+		output = output + " and the VReplication workflow has been deleted."
+	} else {
+		output = output + ". The VReplication workflow hasn't been stopped as the vindex is unowned."
+	}


Won't it always be deleted if there was no error?

mattlord · 2025-01-17T18:54:43Z

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

+	if resp.WorkflowStarted {
+		output = output + " and the VReplication workflow has been started."


IMO it's an error if we internalized it but did not start the workflow. I'm not sure what the point is in that case.

mattlord · 2025-01-17T18:55:34Z

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

 	// vindex has an owner as the lookup vindex will then be
 	// managed by VTGate.
 	externalize.Flags().StringVar(&externalizeOptions.Keyspace, "keyspace", "", "The keyspace containing the Lookup Vindex. If no value is specified then the table-keyspace will be used.")
+	externalize.Flags().BoolVar(&externalizeOptions.Delete, "delete", false, "Delete the VReplication workflow after externalizing LookupVindex, instead of stopping (default false).")


Less ambiguous IMO to say ... workflow after externalizing the VIndex, ...

mattlord · 2025-01-17T19:00:42Z

go/vt/vtctl/workflow/lookup_vindex.go

+		for _, stream := range res.Streams {
+			// All streams need to be frozen.
+			if stream.State != binlogdatapb.VReplicationWorkflowState_Stopped || stream.Message != Frozen {
+				return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "stream %d for %v.%v is not frozen: %v, %v", stream.Id, targetShard.Keyspace(), targetShard.ShardName(), stream.State, stream.Message)


Nit, but keyspace/shard is the standard vs keyspace.shard, suggestion:

return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "stream %d on %v/%v is not frozen: %v, %v", stream.Id, targetShard.Keyspace(), targetShard.ShardName(), stream.State, stream.Message)

mattlord · 2025-01-17T19:03:47Z

go/vt/vtctl/workflow/server.go

+	// Now that we have checked that the vindex has been externalized,
+	// we don't need to delete the write_only parameter from the vindex.


FWIW, this comment is only a little confusing to me. I think we can remove it. If it's helpful for you though fine to keep it as well. I get what you mean, but I think its meaning largely lies in the previous state of the code which is no longer visible (easily).

mattlord · 2025-01-17T19:04:34Z

go/vt/vtctl/workflow/server.go

+		KeepData:         true,
+		KeepRoutingRules: true,
+	}); derr != nil {
+		return nil, vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "failed to delete workflow %s: %v", req.Name, derr)


Nit, but this doesn't feel like a precondition.

go/vt/vtctl/workflow/server.go

mattlord · 2025-01-17T19:07:30Z

go/vt/vtctl/workflow/server.go

 		return nil, err
 	}
 	return resp, s.ts.RebuildSrvVSchema(ctx, nil)
 }

+// LookupVindexInternalize internalizes a lookup vindex. If the vindex has an


I think we should remove this comment now: If the vindex has an // owner then the stopped workflow will also be started.

Since we would fail at `validateExternalized() now, right?

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]>

beingnoble03 requested a review from mattlord January 10, 2025 19:13

mattlord reviewed Jan 12, 2025

View reviewed changes

Check if the vindex has an owner while validating externalized vindex

e866118

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

beingnoble03 force-pushed the lookup-vindex-internalize branch from 2e3db09 to e866118 Compare January 14, 2025 10:26

test: Fix unit tests for LookupVindexComplete

1631cd2

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

beingnoble03 requested a review from mattlord January 14, 2025 10:48

mattlord reviewed Jan 17, 2025

View reviewed changes

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 left a comment

mattlord left a comment

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

mattlord Jan 17, 2025

		if resp.WorkflowStarted {
		output = output + " and the VReplication workflow has been started."

		// Now that we have checked that the vindex has been externalized,
		// we don't need to delete the write_only parameter from the vindex.

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

mattlord left a comment

Choose a reason for hiding this comment

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

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