ingest: release unsent jobs when generateAndSendJob is canceled (#69241) - #70234
ingest: release unsent jobs when generateAndSendJob is canceled (#69241)#70234ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@YangKeao This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthrough
ChangesCancellation Refcount Cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/lightning/backend/local/local_test.go`:
- Around line 2789-2854: Resolve the conflict markers around
TestGenerateAndSendJobDoneAllRefedJobsOnCancel and mockEngineWithData, retaining
the incoming test and surrounding declarations exactly once. Update the test’s
testfailpoint.Enable path to pkg/lightning/backend/local/fakeRegionJobs,
matching the package’s established fakeRegionJobs hook, and remove all conflict
markers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98e8c7f1-6123-4237-a23f-6ef846e2eb37
📒 Files selected for processing (2)
pkg/lightning/backend/local/local.gopkg/lightning/backend/local/local_test.go
| <<<<<<< HEAD:pkg/lightning/backend/local/local_test.go | ||
| // mockEngineWithData is a mock engine that implements common.Engine | ||
| ======= | ||
| func TestGenerateAndSendJobDoneAllRefedJobsOnCancel(t *testing.T) { | ||
| testfailpoint.Enable(t, "github.com/pingcap/tidb/pkg/ingestor/ingestctrl/fakeRegionJobs", "return()") | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| t.Cleanup(cancel) | ||
| local := &Backend{} | ||
| local.WorkerConcurrency.Store(1) | ||
|
|
||
| data := &refCountIngestData{ | ||
| mockIngestData: mockIngestData{ | ||
| {[]byte("b"), []byte("b")}, | ||
| {[]byte("c"), []byte("c")}, | ||
| {[]byte("d"), []byte("d")}, | ||
| {[]byte("e"), []byte("e")}, | ||
| }, | ||
| } | ||
| jobs := []*regionJob{ | ||
| {keyRange: engineapi.Range{Start: []byte("a"), End: []byte("b")}, ingestData: data, region: dummyRegionInfo}, | ||
| {keyRange: engineapi.Range{Start: []byte("b"), End: []byte("c")}, ingestData: data, region: dummyRegionInfo}, | ||
| {keyRange: engineapi.Range{Start: []byte("c"), End: []byte("d")}, ingestData: data, region: dummyRegionInfo}, | ||
| {keyRange: engineapi.Range{Start: []byte("d"), End: []byte("e")}, ingestData: data, region: dummyRegionInfo}, | ||
| } | ||
| jobRange := engineapi.Range{Start: []byte("a"), End: []byte("e")} | ||
| fakeRegionJobs = map[[2]string]struct { | ||
| jobs []*regionJob | ||
| err error | ||
| }{ | ||
| {"a", "e"}: { | ||
| jobs: jobs, | ||
| }, | ||
| } | ||
| t.Cleanup(func() { | ||
| fakeRegionJobs = nil | ||
| }) | ||
|
|
||
| mockEngine := &mockEngineWithData{ | ||
| data: data, | ||
| ranges: []engineapi.Range{jobRange}, | ||
| } | ||
| jobToWorkerCh := make(chan *regionJob) | ||
| var jobWg sync.WaitGroup | ||
| firstJobDone := make(chan struct{}) | ||
|
|
||
| go func() { | ||
| job := <-jobToWorkerCh | ||
| job.done(&jobWg) | ||
| cancel() | ||
| close(firstJobDone) | ||
| }() | ||
|
|
||
| err := local.generateAndSendJob(ctx, mockEngine, int64(config.SplitRegionSize), int64(config.SplitRegionKeys), jobToWorkerCh, &jobWg) | ||
| require.NoError(t, err) | ||
| <-firstJobDone | ||
|
|
||
| require.Eventually(t, func() bool { | ||
| return data.GetRefCount() == 0 | ||
| }, 10*time.Second, 10*time.Millisecond, "ref'd jobs after the canceled send path must all be marked done") | ||
| jobWg.Wait() | ||
| require.Equal(t, int64(0), data.GetRefCount()) | ||
| } | ||
|
|
||
| // mockEngineWithData is a mock engine that implements engineapi.Engine | ||
| >>>>>>> 9f9c4f8b763 (ingest: release unsent jobs when generateAndSendJob is canceled (#69241)):pkg/ingestor/ingestctrl/local_test.go |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve the unreconciled cherry-pick conflict.
<<<<<<<, =======, and >>>>>>> make this package uncompilable. When retaining the incoming test, change its failpoint path to pkg/lightning/backend/local/fakeRegionJobs; the current pkg/ingestor/ingestctrl/... path does not match this package’s established fake-job hook.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/lightning/backend/local/local_test.go` around lines 2789 - 2854, Resolve
the conflict markers around TestGenerateAndSendJobDoneAllRefedJobsOnCancel and
mockEngineWithData, retaining the incoming test and surrounding declarations
exactly once. Update the test’s testfailpoint.Enable path to
pkg/lightning/backend/local/fakeRegionJobs, matching the package’s established
fakeRegionJobs hook, and remove all conflict markers.
Source: Coding guidelines
|
@ti-chi-bot: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ti-chi-bot
Bot
added
cherry-pick-approved
3 participants
This is an automated cherry-pick of #69241
What problem does this PR solve?
Issue Number: close #69240
Problem Summary:
PR #64767 changed
generateAndSendJobto ref all generated region jobs before sending any of them tojobToWorkerCh. That protects shared ingest data from being cleaned too early, but it left a cancellation gap: if the context is canceled after one job is sent and before the remaining jobs in the same slice are sent, only the current unsent job is marked done. Later jobs were already refed but are never handed to a downstream owner and never marked done, sojobWg.Wait()can block forever and keepRunSubtaskstuck.What changed and how does it work?
When
generateAndSendJobobserves context cancellation in the send loop, it now marks every remaining unsent job injobs[i:]as done before returning. Jobs beforeiwere already sent tojobToWorkerChand remain owned by downstream components. The current job and later jobs were not sent, so the generator must balance their earlier refs locally.The new unit test reproduces the stuck path by sending the first generated job, canceling the context, and asserting that
jobWg.Wait()still finishes aftergenerateAndSendJobreturns.Check List
Tests
./tools/check/failpoint-go-test.sh pkg/ingestor/ingestctrl -run TestGenerateAndSendJobDoneAllRefedJobsOnCancel -count=1failed before the fix withjobWg.Wait should finish after generateAndSendJob returns../tools/check/failpoint-go-test.sh pkg/ingestor/ingestctrl -run 'Test(GenerateAndSendJobDoneAllRefedJobsOnCancel|RefAllJobsBeforeSending)$' -count=1make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests