-
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
VReplication: Restore previous minimal e2e test behavior #17016
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17016 +/- ##
==========================================
+ Coverage 67.14% 67.16% +0.02%
==========================================
Files 1571 1571
Lines 251785 251785
==========================================
+ Hits 169049 169108 +59
+ Misses 82736 82677 -59 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <[email protected]>
31b816c
to
f0a233b
Compare
@@ -25,6 +25,14 @@ import ( | |||
|
|||
func testMoveTablesMirrorTraffic(t *testing.T, flavor workflowFlavor) { | |||
setSidecarDBName("_vt") | |||
ogReplicas := defaultReplicas |
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: "og" naming in this function as compared with "orig" naming in other functions. I prefer the latter FWIW.
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 are using this pattern cut-pasted in many places. This is going to be refactored soon into calling a function that changes the number of replica/rdonly count and returns a defer
able reset function. (Or we will remove the globals and make them test-scoped)
So merging as-is for now to get the backport ready well in time for RC2.
Signed-off-by: Matt Lord <[email protected]>
…or (#17016) (#17017) Signed-off-by: Matt Lord <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Description
In #16920 we changed the behavior of the minimal cluster setup so that it went from using a hardcoded value of 0 for the
replica
andrdonly
tablet counts, to instead using the default globals for that (defaultReplicas
,defaultRdonly
): https://github.com/vitessio/vitess/pull/16920/files#diff-127f033346a57c5e68aaa42072891ad18fca2bd1463eb16cd16a23f74b136576R942When working on that PR I noticed that the buffering test started failing every time and the cause was that it now had replicas (and rdonly tablets) and they could not keep up with the load being generated against the primary. So (v)replication could not catch up in time and the
vdiff
did not finish before the 30s timeout. I addressed that in the PR here: a629cbbWhat I didn't notice was that this minimal cluster behavior change also caused the
TestPartialMoveTablesBasic
test to become flaky (not failing every time, but ~ 50% as I watched the last test run on that PR and the subsequent backport PRs). The failures were caused by the same basic problem, we now had replication in place and the replicas could not catch up with the primary fast enough for thevdiff
to complete before the 30s timeout we use in the tests.In this PR we explicitly ensure that the test behavior for users of the
setupMinimalCluster(t)
remains the same after the change noted from #16920 (the exception isTestVtctldclientCLI
in which we explicitly want to have a replica tablet in each shard to test traffic switching). We should backport this to v21 as #16920 was backported to v21.Related Issue(s)
Checklist