Repository navigation
importinto: make dangling import jobs cancellable - #69701
ti-chi-bot[bot] merged 2 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughThis PR adds cancellation handling for IMPORT INTO jobs on user keyspaces that lack a corresponding DXF task ("dangling" jobs). It introduces a ChangesDangling import job cancellation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
size/L
importinto: simplify dangling job cancellation importinto: clarify dangling cancel fallback executor: document import cancel race change docs(importinto): document job cancellation races executor: cancel dangling import jobs change change importinto: test dangling job cancellation importinto: cover dangling cancel fallback
b11c243 to
0e58d40
Compare
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/importer/job.go (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePre-existing typo:
jogStatusCancelledshould bejobStatusCancelled.The constant
jogStatusCancelled(line 57) has a typo — "jog" instead of "job". This is pre-existing, but the newIsCancelledmethod andcancelJobInStatehelper now add new references to it. Consider renaming tojobStatusCancelledfor clarity, though this touches other files that reference the constant.✏️ Optional rename
- jogStatusCancelled = "cancelled" + jobStatusCancelled = "cancelled"Then update all references (including lines 142 and 516 in this file, plus any references in other files).
Also applies to: 142-142, 516-516
🤖 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/executor/importer/job.go` at line 57, The constant name has a typo: `jogStatusCancelled` should be `jobStatusCancelled`, and the new `IsCancelled` method plus `cancelJobInState` helper should use the corrected name for clarity. Rename the constant in `job.go` and update all references in the same file, then sweep any other call sites in the importer package or elsewhere that still refer to `jogStatusCancelled` so everything compiles consistently.
🤖 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.
Nitpick comments:
In `@pkg/executor/importer/job.go`:
- Line 57: The constant name has a typo: `jogStatusCancelled` should be
`jobStatusCancelled`, and the new `IsCancelled` method plus `cancelJobInState`
helper should use the corrected name for clarity. Rename the constant in
`job.go` and update all references in the same file, then sweep any other call
sites in the importer package or elsewhere that still refer to
`jogStatusCancelled` so everything compiles consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: de79b77f-3fe8-46e2-8c82-0e6a9584bcbf
📒 Files selected for processing (15)
pkg/dxf/importinto/BUILD.bazelpkg/dxf/importinto/job.gopkg/dxf/importinto/job_doc.gopkg/dxf/importinto/job_testkit_test.gopkg/dxf/importinto/scheduler.gopkg/dxf/importinto/scheduler_testkit_test.gopkg/executor/BUILD.bazelpkg/executor/import_into.gopkg/executor/import_into_test.gopkg/executor/importer/BUILD.bazelpkg/executor/importer/job.gopkg/executor/importer/job_test.gotests/realtikvtest/configs/next-gen/pd.tomltests/realtikvtest/importintotest3/BUILD.bazeltests/realtikvtest/importintotest3/cross_ks_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69701 +/- ##
================================================
- Coverage 76.3201% 74.2673% -2.0529%
================================================
Files 2041 2054 +13
Lines 560169 581377 +21208
================================================
+ Hits 427522 431773 +4251
- Misses 131746 148867 +17121
+ Partials 901 737 -164
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 7
- Inline comments: 2
- Summary-only findings (no inline anchor): 5
Findings (highest risk first)
🟡 [Minor] (7)
- "cancellable" job-status set is hardcoded independently in three places, and this PR adds a fourth variant (pkg/executor/importer/job.go:136, pkg/executor/importer/job.go:198, pkg/executor/importer/job.go:494, pkg/executor/importer/job.go:499)
- CancelJob doc comment still references a non-existent "paused" job status (pkg/executor/importer/job.go:492)
- New TODO for the highest-priority cancel race has no owner or tracking issue (pkg/executor/import_into.go:417)
- New job-cancelled error text is an undocumented string contract with scheduler.IsCancelledErr (pkg/dxf/importinto/scheduler.go:839, pkg/dxf/framework/scheduler/scheduler.go:812)
- cancelJobInState over-generalizes a two-caller helper into a dynamic-arity SQL builder (pkg/executor/importer/job.go:494, pkg/executor/importer/job.go:499, pkg/executor/importer/job.go:503)
- job_doc.go's design notes are a dangling comment, not attached package documentation (pkg/dxf/importinto/job_doc.go:15, pkg/dxf/importinto/job_doc.go:17)
- Dangling-cancel leaves no durable marker; an old/rolled-back scheduler diverges from the new revert path (pkg/executor/import_into.go:434, pkg/executor/import_into.go:448, pkg/dxf/importinto/scheduler.go:805)
Unanchored findings
🟡 [Minor] (5)
- "cancellable" job-status set is hardcoded independently in three places, and this PR adds a fourth variant
- Request: Introduce one shared
[]string(or small named set) for the "active"/"cancellable" statuses and haveCanCancel,GetActiveJobCnt, and theCancelJob/CancelPendingJobcall sites reference it, so a future status change only needs one edit.
- Request: Introduce one shared
- CancelJob doc comment still references a non-existent "paused" job status
- Request: Update the doc comment on
CancelJobto say "Only a pending/running job can be canceled" to match the states actually passed tocancelJobInState.
- Request: Update the doc comment on
- New TODO for the highest-priority cancel race has no owner or tracking issue
- Request: Attach a tracking issue (or owner) to this TODO so the documented "Highest" risk from
job_doc.goisn't only discoverable by reading source comments.
- Request: Attach a tracking issue (or owner) to this TODO so the documented "Highest" risk from
- New job-cancelled error text is an undocumented string contract with scheduler.IsCancelledErr
- Request: Add a short comment at the
errors.Errorfcall (and/or atIsCancelledErr) stating that the message text is matched byscheduler.IsCancelledErrand must retain thetaskCancelMsgsubstring, or replace the substring match with a typed/sentinel error so the coupling is enforced by the compiler instead of prose.
- Request: Add a short comment at the
- cancelJobInState over-generalizes a two-caller helper into a dynamic-arity SQL builder
- Request: Replace the variadic SQL-builder with two simple, direct UPDATE statements (as
CancelJobhad before), or at minimum drop the runtime placeholder-count loop and keep a fixed two-arg form if a shared helper is still wanted.
- Request: Replace the variadic SQL-builder with two simple, direct UPDATE statements (as
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package importinto |
There was a problem hiding this comment.
🟡 [Minor] job_doc.go's design notes are a dangling comment, not attached package documentation
Impact
The new file's entire content is a single large comment block meant to document the cancel/race invariants for future maintainers ("intentionally kept close to the importinto code").
Because a blank line separates it from package importinto and no declaration follows it, Go's doc-comment attachment rules do not treat it as the package doc: go doc ./pkg/dxf/importinto, pkg.go.dev, and IDE package-overview popups will not surface this content at all.
The existing sibling doc-only files in this codebase (pkg/dxf/example/doc.go, pkg/dxf/framework/doc.go, pkg/dxf/importinto/conflictedkv/doc.go) follow the immediately-attached, no-blank-line convention that this file breaks.
Scope
pkg/dxf/importinto/job_doc.go:15—package importintopkg/dxf/importinto/job_doc.go:17
Evidence
In job_doc.go, line 15 is package importinto, line 16 is blank, and line 17 starts the comment // IMPORT INTO job and DXF task transaction notes.; there is no declaration anywhere in the file for the comment to attach to, so it never becomes discoverable package documentation.
Change request
Remove the blank line so the comment sits directly above package importinto (and consider starting it with // Package importinto ... per Go doc convention), or rename the file to doc.go to match this package family's existing doc-file convention.
| zap.Int64("jobID", jobID), | ||
| zap.String("taskKey", taskKey)) | ||
| failpoint.InjectCall("beforeCancelDanglingImportJob", jobID) | ||
| return cancelDanglingImportJob(ctx, jobID) |
There was a problem hiding this comment.
🟡 [Minor] Dangling-cancel leaves no durable marker; an old/rolled-back scheduler diverges from the new revert path
Impact
The dangling cancel only flips tidb_import_jobs to cancelled and writes nothing durable into the DXF task table; the sole safeguard is the new checkImportJobNotCancelled hook.
During a nextgen rolling upgrade (or after a downgrade), an old SYSTEM-keyspace scheduler owning a task committed after the cancel lacks that hook, so it runs the import to completion: the user is told cancel succeeded and the job row shows cancelled, but the data is fully imported and the DXF task reaches succeed instead of reverted.
Scope
pkg/executor/import_into.go:434—cancelDanglingImportJobpkg/executor/import_into.go:448—CancelPendingJobpkg/dxf/importinto/scheduler.go:805—checkImportJobNotCancelled
Evidence
cancelDanglingImportJob calls CancelPendingJob (job row only) and records no cancel state on the later-committed DXF task. Revert then depends entirely on checkImportJobNotCancelled, added in this PR to OnPrepare/OnNextSubtasksBatch, whose cancel signal is a plain errors.Errorf("import job %d cancelled by user") matched only by the "cancelled by user" substring in IsCancelledErr. An old scheduler binary has neither the hook nor that check, so it neither reverts nor no-ops. The only tests are all-new-cluster (TestCancelDanglingImportJobOnUserKeyspace, TestSchedulerExtLocalSort) and assert TaskStateReverted/0 rows — behavior an old scheduler would not reproduce; no mixed-version or downgrade case is exercised.
Change request
Either version-gate/feature-gate the executor's direct dangling-cancel (or its user-visible success) on the scheduler-side check being present, or record a durable cancel intent that any scheduler version honors. If lockstep-upgrade is the intended contract, state the required upgrade ordering and downgrade window in the PR/release notes and note the mixed-version window is untested.
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joechenrh, wjhuang2016 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: once the present PR merges, I will cherry-pick it on top of release-nextgen-202603 in the new PR and assign it to you. DetailsIn response to this:
Instructions 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. |
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions 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. |
What problem does this PR solve?
Issue Number: ref #61702
Problem Summary:
In nextgen user keyspace mode,
IMPORT INTOcreates the import job and submits the DXF task in separate transactions. If DXF task submission does not complete, the import job can remain without a matching DXF task, andCANCEL IMPORT JOBreportstask not foundinstead of cancelling the dangling import job.There is also a race where a user can cancel the import job between import-job creation and DXF task scheduling. The import scheduler must treat this as cancellation and avoid reverting a legitimate later-running task when multiple scheduler owners observe the same import job.
What changed and how does it work?
This PR makes dangling nextgen import jobs cancellable and hardens import-into scheduler ownership races:
mysql.tidb_import_jobsrow whenCANCEL IMPORT JOBcannot find a matching DXF task by task key.Based on the transaction matrix documented in
pkg/dxf/importinto/job_doc.go, this PR intentionally leaves the following cases for later PRs:CANCEL IMPORT JOBattempts to cancel before the DXF task transaction commits, butWaitTaskDoneByKeylater finds the newly committed task and therefore does not use the dangling-job fallback; that cancel can still wait for the task's natural terminal state.Check List
Tests
./tools/check/failpoint-go-test.sh pkg/dxf/importinto -run 'TestSchedulerExtGlobalSort|TestSubmitTaskNextgen/cancel_dangling_user_keyspace_job_without_DXF_task|TestSchedulerExtLocalSort' -count=1 -tags=intest,deadlock,nextgenSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
IMPORT INTOcancellation handling for next-gen distributed tasks, including better support for jobs that are cancelled before all related work is created.Bug Fixes