Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DO NOT MERGE] traffic mirroring proof-of-concept #14872

Closed
wants to merge 7 commits into from
Closed

[DO NOT MERGE] traffic mirroring proof-of-concept #14872

wants to merge 7 commits into from

Conversation

maxenglander
Copy link
Collaborator

Description

This is a proof-of-concept PR.

Related Issue(s)

#13772

@maxenglander maxenglander added Type: RFC Request For Comment Do Not Merge Skip CI Skip CI actions from running labels Dec 31, 2023
@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Dec 31, 2023
@vitessio vitessio deleted a comment from vitess-bot bot Dec 31, 2023
@github-actions github-actions bot added this to the v19.0.0 milestone Dec 31, 2023
@maxenglander maxenglander removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Dec 31, 2023
@maxenglander maxenglander removed this from the v19.0.0 milestone Dec 31, 2023
@maxenglander maxenglander mentioned this pull request Dec 31, 2023
Closed
7 tasks
@maxenglander maxenglander changed the title [DO NOT REVIEW OR MERGE] traffic mirroring proof-of-concept [DO NOT MERGE] traffic mirroring proof-of-concept Dec 31, 2023
@maxenglander
movetables mirrortraffic
bee9858 
Signed-off-by: Max Englander <[email protected]>
@maxenglander maxenglander force-pushed the maxeng-mirror-routingrules branch from d12e2cf to bee9858 Compare January 3, 2024 06:04
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 3, 2024

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

  • Push additional commits to the associated branch.
  • Remove the stale label.
  • Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

@github-actions github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Feb 3, 2024
@maxenglander maxenglander removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Feb 4, 2024
@maxenglander maxenglander force-pushed the maxeng-mirror-routingrules branch from 8db0226 to b28d2a8 Compare February 4, 2024 03:26
@vitess-bot vitess-bot bot mentioned this pull request Feb 4, 2024
Closed
@maxenglander
merge <- main
af4b77d 
Signed-off-by: Max Englander <[email protected]>
@maxenglander maxenglander force-pushed the maxeng-mirror-routingrules branch from 50a6f4d to c963fd7 Compare February 4, 2024 04:18
@maxenglander
cleanup
8d21447 
Signed-off-by: Max Englander <[email protected]>
@maxenglander maxenglander force-pushed the maxeng-mirror-routingrules branch from c963fd7 to 8d21447 Compare February 4, 2024 04:29
maxenglander
maxenglander commented Feb 4, 2024
View reviewed changes
go/vt/vtgate/engine/plan.go
@@ -37,7 +38,7 @@ type Plan struct {
Instructions Primitive // Instructions contains the instructions needed to fulfil the query.
BindVarNeeds *sqlparser.BindVarNeeds // Stores BindVars needed to be provided as part of expression rewriting
Warnings []*query.QueryWarning // Warnings that need to be yielded every time this query runs
TablesUsed []string // TablesUsed is the list of tables that this plan will query
TablesUsed sqlparser.TableNames // TablesUsed is the list of tables that this plan will query
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was made to enable planbuilder to figure out which keyspaces the original plan touches, and use that information to figure out whether/how to build a mirror plan, without having to do strings.Split.

maxenglander
maxenglander commented Feb 4, 2024
View reviewed changes
go/vt/vtgate/planbuilder/builder.go
engine.NewPercentMirrorTarget(percent, target.primitive),
)
tables := append(from.tables, target.tables...)
operators.SortTableNames(tables)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT we only need table names to be sorted for logging/stats purposes, e.g. so that planbuilder tests are consistent. Maybe we can delay sorting until it's needed?

@maxenglander
cleanup
6eb2e70 
Signed-off-by: Max Englander <[email protected]>
systay
systay reviewed Feb 8, 2024
View reviewed changes
go/vt/vtgate/engine/plan.go
Comment on lines +82 to +86
if table.Qualifier.NotEmpty() {
tablesUsed[i] = fmt.Sprintf("%s.%s", table.Qualifier.String(), table.Name.String())
} else {
tablesUsed[i] = table.Name.String()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if table.Qualifier.NotEmpty() {
tablesUsed[i] = fmt.Sprintf("%s.%s", table.Qualifier.String(), table.Name.String())
} else {
tablesUsed[i] = table.Name.String()
}
tablesUsed[i] = sqlparser.String(table)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@systay the problem with that is it adds backticks around keyspace names and tables. Which I guess isn't really a problem if we're OK with updating all the planbuilder tests and any other tests that use this info?

systay
systay reviewed Feb 8, 2024
View reviewed changes
go/vt/vtgate/planbuilder/plancontext/vschema.go
HasMirrorRules() bool
}

// mirrorVSchema is a wrapper which returns mirrored versions of values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move mirrorVSchema to it's own file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@systay systay systay left review comments

@deepthi deepthi Awaiting requested review from deepthi deepthi will be requested when the pull request is marked ready for review deepthi is a code owner

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal will be requested when the pull request is marked ready for review harshit-gangal is a code owner

@mattlord mattlord Awaiting requested review from mattlord mattlord will be requested when the pull request is marked ready for review mattlord is a code owner

@ajm188 ajm188 Awaiting requested review from ajm188 ajm188 will be requested when the pull request is marked ready for review ajm188 is a code owner

@notfelineit notfelineit Awaiting requested review from notfelineit notfelineit will be requested when the pull request is marked ready for review notfelineit is a code owner

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps rohit-nayak-ps will be requested when the pull request is marked ready for review rohit-nayak-ps is a code owner

@frouioui frouioui Awaiting requested review from frouioui frouioui will be requested when the pull request is marked ready for review frouioui is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100 GuptaManan100 will be requested when the pull request is marked ready for review GuptaManan100 is a code owner

@arthurschreiber arthurschreiber Awaiting requested review from arthurschreiber arthurschreiber will be requested when the pull request is marked ready for review arthurschreiber is a code owner

Assignees
No one assigned
Labels
Do Not Merge Skip CI Skip CI actions from running Type: RFC Request For Comment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxenglander @systay