Skip to content

dxf: derive current keyspace when acquiring task runtime - #69434

Merged
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
D3Hunter:derive-ks-runtime
Jun 24, 2026
Merged

ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
D3Hunter:derive-ks-runtime

Conversation

@D3Hunter

@D3Hunter D3Hunter commented Jun 24, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #69182

Problem Summary:

DXF scheduler and taskexecutor runtime acquisition used the manager store keyspace as the current keyspace. When a task session is created for the runtime lookup, the actual current keyspace should come from that session's store so cross-keyspace runtime acquisition is based on the session that is doing the lookup.

What changed and how does it work?

  • Derive the current keyspace inside dxfutil.AcquireTaskRuntime from se.GetStore().GetKeyspace().
  • Remove the current-keyspace parameter from scheduler and taskexecutor call sites.
  • Add a shared dxfutil.GenHolderID helper for scheduler and executor runtime holder IDs.
  • Update DXF runtime acquisition tests so mocked sessions expose the expected keyspace store.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Unit test commands:

make bazel_prepare
go test -run TestAcquireTaskRuntime -tags=intest,deadlock -count=1 ./pkg/dxf/framework/dxfutil
./tools/check/failpoint-go-test.sh pkg/dxf/framework/scheduler -run TestStartSchedulerCrossKeyspaceRuntime -count=1
./tools/check/failpoint-go-test.sh pkg/dxf/framework/taskexecutor -run 'TestStartTaskExecutor(CrossKeyspaceRuntime|ResolveTaskRuntime(FromTaskKeyspace|Error))' -count=1
make lint

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

  • New Features

    • Task runtime acquisition now derives “current keyspace” from the active session to select the correct runtime automatically.
    • Introduced a standardized holder ID format for task runtime acquisition across scheduler and executor flows.
  • Bug Fixes

    • Improved cross-keyspace runtime selection to better align task execution with the active session context.
    • Updated scheduler and task-executor runtime acquisition to use the standardized holder ID generation consistently.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 779d7996-4f35-4ce5-a69d-009c906fd56c

📥 Commits

Reviewing files that changed from the base of the PR and between 7f796e4 and 29c0599.

📒 Files selected for processing (5)
  • pkg/dxf/framework/dxfutil/util.go
  • pkg/dxf/framework/scheduler/BUILD.bazel
  • pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go
  • pkg/dxf/framework/taskexecutor/BUILD.bazel
  • pkg/dxf/framework/taskexecutor/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/dxf/framework/dxfutil/util.go
  • pkg/dxf/framework/taskexecutor/manager_test.go

📝 Walkthrough

Walkthrough

AcquireTaskRuntime now derives the current keyspace from the session store, GenHolderID centralizes holder ID formatting, and scheduler/task-executor runtime setup and tests are updated to use the new API shape.

Changes

AcquireTaskRuntime Refactor

Layer / File(s) Summary
dxfutil API and tests
pkg/dxf/framework/dxfutil/util.go, pkg/dxf/framework/dxfutil/util_test.go
AcquireTaskRuntime now reads the current keyspace from the session store inside WithNewSession, and GenHolderID returns DXF/<component>/<taskID>. The dxfutil tests now seed session stores with explicit keyspaces and updated call arguments.
Scheduler and task-executor runtime wiring
pkg/dxf/framework/scheduler/scheduler_manager.go, pkg/dxf/framework/taskexecutor/manager.go
startScheduler and startTaskExecutor switch holder ID formatting to GenHolderID and update AcquireTaskRuntime calls to the new argument list. Both files remove the fmt import.
Scheduler and task-executor test session setup
pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go, pkg/dxf/framework/taskexecutor/manager_test.go, pkg/dxf/framework/scheduler/BUILD.bazel, pkg/dxf/framework/taskexecutor/BUILD.bazel
Scheduler and task-executor tests now initialize session Store values before invoking session callbacks, and their holder ID expectations use GenHolderID. The scheduler and task-executor Bazel test deps add dxfutil.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • pingcap/tidb#68824: Updates task-execution keyspace wiring in taskexecutor/manager.go, which is directly adjacent to the caller changes in this PR.
  • pingcap/tidb#68890: Changes cross-keyspace runtime acquisition and holder-ID bookkeeping that this PR’s AcquireTaskRuntime refactor builds on.
  • pingcap/tidb#69065: Covers the same scheduler/task-executor runtime acquisition flow that this PR continues with the new helper and signature.

Suggested labels

size/XL, release-note

Suggested reviewers

  • joechenrh
  • wjhuang2016

Poem

🐇 Hop, hop, the keyspace now appears,
From session stores instead of passed-through gears.
GenHolderID keeps the names aligned,
Scheduler and executor stay neatly twined.
One less arg, the rabbit taps its paw,
And runtime paths now follow one clear law.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: deriving the current keyspace during task runtime acquisition.
Description check ✅ Passed The description matches the template with issue reference, problem summary, implementation details, tests, and release note sections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.4967%. Comparing base (b53834a) to head (29c0599).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #69434        +/-   ##
================================================
- Coverage   76.3177%   74.4967%   -1.8210%     
================================================
  Files          2041       2042         +1     
  Lines        561171     574351     +13180     
================================================
- Hits         428273     427873       -400     
- Misses       131997     146094     +14097     
+ Partials        901        384       -517     
Flag Coverage Δ
integration 40.9460% <0.0000%> (+1.3180%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4610% <ø> (ø)
parser ∅ <ø> (∅)
br 47.6221% <ø> (-15.1290%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@D3Hunter

Copy link
Copy Markdown
Contributor Author

/retest

@ingress-bot

Copy link
Copy Markdown

🔍 Starting code review for this PR...

@ingress-bot ingress-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.

Summary

  • Total findings: 4
  • Inline comments: 4
  • Summary-only findings (no inline anchor): 0
Findings (highest risk first)

🟡 [Minor] (2)

  1. AcquireTaskRuntime doc omits the implicit store-keyspace precondition on sessionProvider (pkg/dxf/framework/dxfutil/util.go:31, pkg/dxf/framework/dxfutil/util.go:40)
  2. GenHolderID extraction is incomplete: test holderID() helpers and inline literals still duplicate the format policy (pkg/dxf/framework/dxfutil/util.go:88, pkg/dxf/framework/taskexecutor/manager_test.go:252, pkg/dxf/framework/taskexecutor/manager_test.go:682, pkg/dxf/framework/taskexecutor/manager_test.go:736, pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go:293)

🧹 [Nit] (2)

  1. GenHolderID doc comment uses infinitive verb and omits the component parameter from the description (pkg/dxf/framework/dxfutil/util.go:87)
  2. se.Store assignment in expectRuntimeFromNewSession lacks a comment explaining its role in keyspace routing (pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go:76, pkg/dxf/framework/taskexecutor/manager_test.go:72)

Comment thread pkg/dxf/framework/dxfutil/util.go
Comment thread pkg/dxf/framework/dxfutil/util.go
Comment thread pkg/dxf/framework/dxfutil/util.go Outdated
server.EXPECT().GetRuntime().Return(runtime).AnyTimes()
taskMgr.EXPECT().WithNewSession(gomock.Any()).DoAndReturn(func(fn func(sessionctx.Context) error) error {
se := utilmock.NewContext()
se.Store = runtime.Store()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 [Nit] se.Store assignment in expectRuntimeFromNewSession lacks a comment explaining its role in keyspace routing

The added se.Store = runtime.Store() line controls which branch of AcquireTaskRuntime executes: it makes the mock session's store carry the same keyspace as the runtime, so the same-keyspace path is taken and GetRuntime() is called. Without this context, the assignment reads as boilerplate rather than a contract-critical setup.

@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 24, 2026
@D3Hunter

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@D3Hunter

Copy link
Copy Markdown
Contributor Author

/retest

@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 24, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-24 08:45:39.201050764 +0000 UTC m=+110022.864276277: ☑️ agreed by joechenrh.
  • 2026-06-24 11:18:56.302769629 +0000 UTC m=+119219.965995142: ☑️ agreed by wjhuang2016.

@ti-chi-bot
ti-chi-bot Bot merged commit 42edcdf into pingcap:master Jun 24, 2026
34 checks passed
@D3Hunter
D3Hunter deleted the derive-ks-runtime branch June 25, 2026 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants