VReplication: Align VReplication and VTGate VStream Retry Logic#17783
VReplication: Align VReplication and VTGate VStream Retry Logic#17783mattlord merged 6 commits intovitessio:mainfrom
Conversation
Signed-off-by: Matt Lord <mattalord@gmail.com>
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
|
47ef9af to
b031939
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
b031939 to
8f31754
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17783 +/- ##
==========================================
- Coverage 67.95% 67.45% -0.50%
==========================================
Files 1586 1592 +6
Lines 255208 258086 +2878
==========================================
+ Hits 173423 174093 +670
- Misses 81785 83993 +2208 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
6f2a0cc to
dc93152
Compare
dc93152 to
4ef0558
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
go/vt/vtgate/vstream_manager.go
Outdated
| return false, false | ||
| // For anything else, if this is a recoverable/ephemeral error -- such as a | ||
| // MAX_EXECUTION_TIME SQL error during the copy phase -- then retry. | ||
| return !vreplication.IsUnrecoverableError(err), false |
There was a problem hiding this comment.
Should we move this to its own package or maybe to like vterrors so we don't pull in all of the vreplication package here?
There was a problem hiding this comment.
I don't mind doing that, but from when I've looked at the binaries built in cases like this the binary only includes the necessary symbols.
Maybe you're thinking more about the package level management than the actual binaries though?
There was a problem hiding this comment.
Maybe you're thinking more about the package level management than the actual binaries though?
More from a design perspective, but also even if it's not in the final binary, it stil compiles a lot more etc. And I think these are cases where having its own or putting it in a smaller more focused package is useful to do. It also makes it easier to reuse by avoiding things like import loops in the future which would force the same refactor then anyway too.
There was a problem hiding this comment.
No disagreement there. It's not quite general purpose, but I can move it to vterrors or somewhere else and include VReplication in the name.
There was a problem hiding this comment.
I was going to create a subdir and vreplerror package but after thinking about it more... we don't need to re-use the same function in the vstream manager as our context is a little different (e.g. it's read-only SELECTs in that context so a much smaller set of relevant errors). So I switched over to using a similar, but more generic, function that I had created for this purpose some time back: 7c5c747
Signed-off-by: Matt Lord <mattalord@gmail.com>
…ssio#17783) Signed-off-by: Thomas Thornton <tthornton@salesforce.com>
* VStream API: validate that last PK has fields defined (vitessio#16478) Signed-off-by: Thomas Thornton <tthornton@salesforce.com> * Remove go/vt/vttablet/tabletserver/vstreamer/vstreamer_test.go * VReplication: Align VReplication and VTGate VStream Retry Logic (vitessio#17783) Signed-off-by: Thomas Thornton <tthornton@salesforce.com> * Fix import * Undo import changes --------- Signed-off-by: Thomas Thornton <tthornton@salesforce.com> Co-authored-by: Thomas Thornton <tthornton@salesforce.com>
Description
In VTGate VStreams, we've been returning errors to the client in various cases where a retry is likely to succeed. The most egregious example is the query interrupted MySQL error (see issue) that we expect to get from each tablet during the copy phase — when multiple copy phase cycles are required — every
vreplication_copy_phase_duration. We expect that for large tables in Vitess v19.0 and later after the work done in #13840 .This PR addresses this case and a variety of others by leveraging retry logic that is similar to what we use for vreplication streams.
Related Issue(s)
Checklist