-
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 (queryserving) #15992
add support for vtgate traffic mirroring (queryserving) #15992
Conversation
Signed-off-by: Max Englander <[email protected]>
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
|
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15992 +/- ##
==========================================
+ Coverage 68.87% 68.88% +0.01%
==========================================
Files 1562 1564 +2
Lines 200624 201012 +388
==========================================
+ Hits 138179 138470 +291
- Misses 62445 62542 +97 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Max Englander <[email protected]>
Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
083c9e1
to
9ed0b03
Compare
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
9ed0b03
to
2a786e5
Compare
Signed-off-by: Max Englander <[email protected]>
mi := MirrorInfo{} | ||
for _, t := range tableInfos { | ||
if mr := t.GetMirrorRule(); mr != nil { | ||
if mi.Percent == 0 || mr.Percent < mi.Percent { | ||
mi.Percent = mr.Percent | ||
} | ||
} |
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.
we can specifically check for RealTable type here and check on the mirror rule.
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.
are you saying we can remove the GetMirrorRule
method from TableInfo
and just directly check for t.(*RealTable)
?
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, we can type cast it and check the mirror rule
cloneMirror := *m | ||
cloneMirror.Percent = m.Percent | ||
cloneMirror.Operator = inputs[0] | ||
cloneMirror.Target = inputs[1] | ||
return &cloneMirror | ||
} | ||
|
||
// Inputs returns the inputs for this operator | ||
func (m *PercentBasedMirror) Inputs() []Operator { | ||
return []Operator{ | ||
m.Operator, | ||
m.Target, | ||
} | ||
} | ||
|
||
// SetInputs changes the inputs for this op | ||
func (m *PercentBasedMirror) SetInputs(inputs []Operator) { | ||
m.Operator = inputs[0] | ||
m.Target = inputs[1] | ||
} |
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.
nit: better to check the slice length before using indices.
panic("not supported") | ||
} | ||
|
||
func (m *PercentBasedMirror) AddColumn(ctx *plancontext.PlanningContext, reuseExisting bool, addToGroupBy bool, expr *sqlparser.AliasedExpr) int { | ||
panic("not supported") | ||
} |
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.
nit: for all the panic send along and error inside, like panic(vterrors.VT13001(...))
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.
adding a test with mirror percentage 0, should be a good one to check plan without mirror operator.
"from_table": "unsharded_src1.t1", | ||
"to_table": "unsharded_dst1.t1", | ||
"percent": 50 | ||
}, | ||
{ | ||
"from_table": "unsharded_src1.t2", | ||
"to_table": "sharded_dst1.t1", | ||
"percent": 50 | ||
}, | ||
{ | ||
"from_table": "unsharded_src2.t1", | ||
"to_table": "unsharded_src2.t1", | ||
"percent": 50 | ||
}, | ||
{ | ||
"from_table": "unsharded_src2.t2", |
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.
having variable percentage would be a good to have, also to verify which percentage the plan selects
…s, return mirror keyspace Signed-off-by: Max Englander <[email protected]>
… case Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
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.
PR is looking good, added some more comments based on the changes.
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Co-authored-by: Harshit Gangal <[email protected]> Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Not sure why DCO check is still pending. I assume all the commits are correctly signed off. @maxenglander if you can confirm that, we can bypass and merge instead of a force-push -> Run CI all over again -> merge. |
Hey @deepthi all the commits have the "Verified" sign on GH, and each commit individually has the |
Description
Query-serving component for #15945.
Related Issue(s)
#13772
#15945
Checklist
Deployment Notes