-
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
Fix for premature buffering #17013
base: main
Are you sure you want to change the base?
Fix for premature buffering #17013
Conversation
Signed-off-by: Manan Gupta <[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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17013 +/- ##
==========================================
+ Coverage 67.04% 67.15% +0.11%
==========================================
Files 1571 1571
Lines 251677 251804 +127
==========================================
+ Hits 168729 169105 +376
+ Misses 82948 82699 -249 ☔ View full report in Codecov by Sentry. |
return | ||
} | ||
// We're running queries every 100 millisecond and verifying the results are all correct. | ||
res, err := conn.ExecuteFetch(utils.GetSelectionQuery(), rowCount+10, false) |
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.
So this is only running SELECT
queries during the failover? SELECT
queries don't get buffered, and don't fail if the server is put into read_only
mode during the failover. You'll need to run DML
queries to actually see any impact. 🤔
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.
Replica reads aren't buffered, but any query including SELECTs that is target to a PRIMARY tablet are buffered. But yes, the read_only
wouldn't really fail for SELECTs. I'll try DMLs instead 👍
for _, shard := range shards { | ||
wg.Add(1) | ||
go func() { | ||
time.Sleep(time.Second * time.Duration(rand.IntN(6))) |
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.
You might have to remove this time.Sleep
here. I don't have a good idea of how long the failovers take in the tests, but I'd expect them to take around a second or so, which means it's very unlikely to happen at the same time.
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.
I had tried with and without the sleep. I actually made a few more changes here and there, but nothing seemed to work. I'll try what you suggested in 👇
It might be easier to reproduce this via an "external" failover, as it's simpler to get the sequencing right:
|
Thanks Arthur! I was able to reproduce the problem now! |
Signed-off-by: Manan Gupta <[email protected]>
Description
Related Issue(s)
Checklist
Deployment Notes