[release-23.0] VTGate: fix warming reads timeout context (#19674)#19728
Conversation
|
Hello @timvaillancourt, there are conflicts in this backport. Please address them in order to merge this Pull Request. You can execute the snippet below to reset your branch and resolve the conflict manually. Make sure you replace |
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
900f8a9 to
c67bb6f
Compare
|
Resolved conflicts for backport of #19674 to release-23.0. Conflicting files:
Reverted: |
There was a problem hiding this comment.
Pull request overview
Backport of #19674 to release-23.0 to ensure VTGate “warming reads” use a dedicated, correctly-scoped timeout context that starts when the warming goroutine runs and is not canceled when the primary request finishes.
Changes:
- Introduces
VCursor.WarmingReadsContext(ctx)to create a timeout-bounded, parent-detached context (and cancel func) for warming reads. - Updates warming read execution to use the warming context for routing and execution, and to avoid unnecessary cloning when the warming channel is full.
- Adds targeted tests covering timeout behavior, parent-cancel independence, and channel-full dropping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go/vt/vtgate/executorcontext/vcursor_impl.go | Moves warming timeout/context creation into WarmingReadsContext, fixes cancel handling, and updates replica-warming clone setup. |
| go/vt/vtgate/engine/route.go | Uses WarmingReadsContext inside the warming goroutine and clones only after acquiring a channel slot. |
| go/vt/vtgate/engine/primitive.go | Extends VCursor interface with WarmingReadsContext and documents intended usage. |
| go/vt/vtgate/engine/fake_vcursor_test.go | Updates test vcursor helpers to satisfy the extended VCursor interface and adds a ResolveDestinations hook. |
| go/vt/vtgate/engine/route_warming_reads_test.go | Adds new tests validating warming context behavior (deadline, independence from parent cancel, dropping on full channel, deadline exceeded). |
| return context.WithTimeout(context.Background(), vc.warmingReadsTimeout) | ||
| } | ||
|
|
||
| func TestWarmingReadsSkipsForUpdate(t *testing.T) { |
There was a problem hiding this comment.
Test name TestWarmingReadsSkipsForUpdate doesn’t match what the test actually validates (context deadline independence / propagation and execution across multiple query shapes, including a regular SELECT). Renaming this test to reflect its assertions would make failures easier to interpret.
| func TestWarmingReadsSkipsForUpdate(t *testing.T) { | |
| func TestWarmingReadsContextDeadlineAndQueryShapes(t *testing.T) { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-23.0 #19728 +/- ##
=================================================
- Coverage 69.45% 45.70% -23.76%
=================================================
Files 1607 67 -1540
Lines 215090 7181 -207909
=================================================
- Hits 149397 3282 -146115
+ Misses 65693 3899 -61794
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This is a backport of #19674