-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
add support for vtgate traffic mirroring #15945
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15945 +/- ##
==========================================
- Coverage 68.71% 68.70% -0.02%
==========================================
Files 1544 1547 +3
Lines 198011 198247 +236
==========================================
+ Hits 136064 136200 +136
- Misses 61947 62047 +100 ☔ View full report in Codecov by Sentry. |
f3762c8
to
aed6da5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too big a PR to review.
Can we break this into 2 parts?
- Vreplication changes
- Query Serving changes
@harshit-gangal can do! |
Signed-off-by: Max Englander <max@planetscale.com>
aed6da5
to
79983de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, excited to see this!
Signed-off-by: Max Englander <max@planetscale.com>
Signed-off-by: Max Englander <max@planetscale.com>
606155e
to
0a97a1a
Compare
Signed-off-by: Max Englander <max@planetscale.com>
0a97a1a
to
a9dbf34
Compare
go/cmd/vtctldclient/command/vreplication/common/mirrortraffic.go
Outdated
Show resolved
Hide resolved
go/cmd/vtctldclient/command/vreplication/common/mirrortraffic.go
Outdated
Show resolved
Hide resolved
go/cmd/vtctldclient/command/vreplication/common/mirrortraffic.go
Outdated
Show resolved
Hide resolved
}, 25) | ||
|
||
mt.SwitchWrites() | ||
confirmNoMirrorRules(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does switching traffic delete mirror rules? Interesting, but also seems correct. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i think you suggested that during one of the Vitess team meetings i attended, unless i misunderstood 😅
MirrorRules: &vschemapb.MirrorRules{ | ||
Rules: []*vschemapb.MirrorRule{}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for people to provide MirrorRules through ApplyVSchema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, it should not be possible any more than it is possible to change routing rules through ApplyVSchema
.
Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Max Englander <max.englander@gmail.com>
Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Max Englander <max.englander@gmail.com>
Signed-off-by: Max Englander <max@planetscale.com>
Signed-off-by: Max Englander <max@planetscale.com>
c5f6583
to
446445c
Compare
Signed-off-by: Max Englander <max@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This is going to be good :)
* cherry pick of 15945 * fix conflicts Signed-off-by: Max Englander <max@planetscale.com> --------- Signed-off-by: Max Englander <max@planetscale.com> Co-authored-by: Max Englander <max@planetscale.com>
Description
This PR is split into two:
Add support for cross-keyspace traffic mirroring.
ApplyMirrorRules
orMoveTables MirrorTraffic
.Mirror
engine primitive during the query planning stage, which will contain the original query plan, as well as a "mirror target" plan.Mirror
primitive, it will execute the original plan and mirror target in parallel. If the mirror target plan takes longer than the original plan, it is cancelled.Limitations
There are a lot of limitations, restrictions, and "missing features" in the initial implementation. Some of the restrictions can be relaxed if demand for new use cases arise, and missing features can be added in future PRs:
SELECT
andUNION
statements can be mirrored at this time.from_table
andto_table
), circular (ks1
=>ks2
=>ks1
), or chained (ks1
=>ks2
=>ks3
).MirrorTraffic
can only be used on simple, forward-directionMoveTables
workflows.MirrorTraffic
is not supported, for partial, multi-tenant, reverse, or non-Move Tables
workflows at this time.MirrorTraffic
must be called afterMoveTables Create
, and beforeMoveTables SwitchTraffic
.Benchmarks
When mirror rules are defined, memory allocations increase and performance decreases.
Plan builder
During the planning phase, when producing a mirror plan, memory allocations increase while performance decreases compared to when valid mirrors are not defined. This is because, when a mirror plan is produced, the original statement is cloned, and
buildRoutePlan
is run twice: once for the original statement, and once for the cloned statement.Note: the times below measure how long it takes to run the entire sub-suite of mirror cases defined in
mirror_cases.json
.Executor
Likewise, during the query execution phase, memory allocations increase and performance decreases with higher percentages of traffic are mirrored.
E2E
E2E, synthetic
TODO: spin up a realistic Vitess cluster with two keyspaces, and compare baseline performance to performance when mirror rules are configured.
Related Issue(s)
Addresses #13772.
Checklist
Deployment Notes