Skip to content

dxf, domain: acquire cross-keyspace runtimes at manager boundaries#69023

Open
D3Hunter wants to merge 3 commits into
pingcap:masterfrom
D3Hunter:crossks-cleanup
Open

dxf, domain: acquire cross-keyspace runtimes at manager boundaries#69023
D3Hunter wants to merge 3 commits into
pingcap:masterfrom
D3Hunter:crossks-cleanup

Conversation

@D3Hunter

@D3Hunter D3Hunter commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #68883

Problem Summary:

NextGen DXF work can run IMPORT and distributed DDL backfill tasks against user keyspaces from the SYSTEM keyspace. Those task paths need explicit ownership of cross-keyspace runtimes so stores, session pools, schema syncers, etcd clients, and background loops are retained only while the DXF scheduler or executor is actively using them.

What changed and how does it work?

This PR routes task-keyspace runtime access through the DXF manager boundaries:

  • Extend sqlsvrapi.Server with GetRuntime() and make acquired keyspace handles expose the shared Runtime view.
  • Acquire cross-keyspace runtime handles in the DXF scheduler manager and task executor manager with stable bookkeeper IDs:
    • DXF/scheduler/<taskID>
    • DXF/executor/<taskID>
  • Pass the resolved TaskRuntime into schedulers and task executors, and release acquired handles on normal exit, init failure, missing executor factory, and acquisition failure paths.
  • Update IMPORT and distributed DDL backfill code to consume the manager-provided runtime store/session pool instead of performing extra raw keyspace store/session-pool lookups.
  • Add sqlsvrapi gomock targets and focused tests for runtime acquisition/release boundaries, failure cleanup, runtime keyspace validation, and IMPORT/DDL consumers.

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.
make bazel_prepare
./tools/check/failpoint-go-test.sh pkg/dxf/framework/scheduler -run 'TestStartSchedulerAcquiresCrossKeyspaceRuntimeAndReleasesOnExit|TestStartSchedulerReleasesCrossKeyspaceRuntimeOnInitFailure|TestStartSchedulerStopsWhenCrossKeyspaceRuntimeAcquireFails' -count=1 -tags=intest,deadlock,nextgen
./tools/check/failpoint-go-test.sh pkg/dxf/framework/taskexecutor -run 'TestStartTaskExecutorAcquiresCrossKeyspaceRuntimeAndReleasesOnExit|TestStartTaskExecutorReleasesCrossKeyspaceRuntimeOnInitFailure|TestStartTaskExecutorStopsWhenCrossKeyspaceRuntimeAcquireFails|TestStartTaskExecutorReleasesCrossKeyspaceRuntimeOnMissingFactory|TestStartTaskExecutorResolveTaskRuntimeFromTaskKeyspace|TestStartTaskExecutorResolveTaskRuntimeError|TestBaseTaskExecutorInitChecksTaskRuntimeKeyspace' -count=1 -tags=intest,deadlock,nextgen
./tools/check/failpoint-go-test.sh pkg/domain/crossks -run 'TestAcquireRuntimeHandle|TestEvictRuntime|TestRuntimeHandleManagerCloseClosesAllEntriesRegardlessOfIdleTimeout|TestGCLoopExitsWhenContextCancelled|TestDomainAcquireKSRuntimeHandle' -count=1 -tags=intest,deadlock,nextgen
./tools/check/failpoint-go-test.sh pkg/dxf/importinto -run 'TestImportInto/(TestSchedulerInit|TestGetTaskMgrForAccessingImportJobUsesTaskRuntime)$|TestImportTaskExecutor$|TestImportTaskExecutorUsesTaskRuntimeStoreWithoutExtraLookup$' -count=1 -tags=intest,deadlock,nextgen
./tools/check/failpoint-go-test.sh pkg/ddl -run 'TestBackfillingSchedulerLocalMode|TestBackfillingSchedulerGlobalSortMode' -count=1 -tags=intest,deadlock,nextgen
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

  • Refactor
    • Unified runtime abstraction across schedulers, executors, import and DDL flows for consistent keyspace validation and safer runtime/session management, improving resource cleanup on failures.
  • Tests
    • Added extensive mock-backed tests and generated test mocks to validate cross-keyspace runtime acquisition, release semantics, and keyspace-consistency checks.

dxf: route crossks runtime through task params

tests: cover crossks runtime reclamation

docs/design: record crossks runtime validation

bazel

change

change

change

change test

change test using mock
@ti-chi-bot ti-chi-bot Bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 8, 2026
@pantheon-ai

pantheon-ai Bot commented Jun 8, 2026

Copy link
Copy Markdown

@D3Hunter I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot

ti-chi-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign d3hunter for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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 the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 8, 2026
@tiprow

tiprow Bot commented Jun 8, 2026

Copy link
Copy Markdown

Hi @D3Hunter. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

@coderabbitai

coderabbitai Bot commented Jun 8, 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: 8520b4af-8e2a-431e-9047-92e291cca2d0

📥 Commits

Reviewing files that changed from the base of the PR and between f5ffb69 and 7c8e515.

📒 Files selected for processing (2)
  • pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go
  • pkg/dxf/importinto/scheduler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go

📝 Walkthrough

Walkthrough

This PR refactors the SQL server runtime abstraction by introducing sqlsvrapi.Runtime (Store(), SysSessionPool()), replacing TaskStore/SessPool usage with TaskRuntime/SysSessionPool across scheduler, task executor, importinto, and DDL backfilling; it adds mocks, BUILD updates, dxfutil.CheckRuntime, and tests for acquisition/release and keyspace validation.

Changes

Core runtime abstraction and domain updates

Layer / File(s) Summary
Runtime contract and Domain changes
pkg/domain/sqlsvrapi/server.go, pkg/domain/domain.go
Adds Runtime interface (Store(), SysSessionPool()), Domain.GetRuntime(), renames internal session-pool helper, and starts system-keyspace GC loop.
Cross-keyspace accessor rename
pkg/domain/crossks/cross_ks.go, pkg/domain/crossks/*_test.go
Replaces SessPool() with SysSessionPool() on runtime handles and session manager; updates tests accordingly.

Mock infrastructure generation and build

Layer / File(s) Summary
Makefile mock target and generated mocks
Makefile, pkg/domain/sqlsvrapi/mock/*
gen_mock now generates gomock files for Server, Runtime, KSRuntimeHandle; mock package BUILD added listing generated sources.
Build updates for consumers
pkg/dxf/.../BUILD.bazel, pkg/dxf/framework/taskexecutor/BUILD.bazel, pkg/ddl/BUILD.bazel, pkg/dxf/importinto/BUILD.bazel
Test/library deps updated to include //pkg/domain/sqlsvrapi and //pkg/domain/sqlsvrapi/mock where needed; shard_count adjustments for tests.

DDL backfilling scheduler and executor

Layer / File(s) Summary
DDL backfill runtime adoption
pkg/ddl/backfilling_dist_scheduler.go, pkg/ddl/backfilling_dist_executor.go
Backfill scheduler/executor obtain store and session pool from sch.TaskRuntime/KSRuntimeHandle; getUserTableFromTaskStore rebuilt from metadata with autoid allocators; tests wired to mocked runtime.
DDL test build wiring
pkg/ddl/BUILD.bazel, pkg/ddl/backfilling_dist_scheduler_test.go
Adds mock dependency and test helpers to inject mocked runtimes into backfilling scheduler tests.

DXF scheduler framework refactor

Layer / File(s) Summary
Scheduler Param contract
pkg/dxf/framework/scheduler/interface.go
Param gains TaskRuntime sqlsvrapi.Runtime, removes TaskStore; NewParamForTest signature updated.
Scheduler init and manager
pkg/dxf/framework/scheduler/scheduler.go, pkg/dxf/framework/scheduler/scheduler_manager.go
BaseScheduler.Init() validates keyspace via dxfutil.CheckRuntime; manager acquires/releases per-keyspace runtimes and passes TaskRuntime into schedulers; tests updated.
Scheduler tests
pkg/dxf/framework/scheduler/*_test.go
Adds runtime/session mock helpers and tests for cross-keyspace runtime acquisition/release scenarios.

DXF taskexecutor framework refactor

Layer / File(s) Summary
TaskExecutor Param and validation
pkg/dxf/framework/taskexecutor/task_executor.go
Param uses TaskRuntime sqlsvrapi.Runtime; BaseTaskExecutor.Init() uses dxfutil.CheckRuntime.
TaskExecutor manager and tests
pkg/dxf/framework/taskexecutor/manager.go, pkg/dxf/framework/taskexecutor/manager_test.go
Manager acquires/releases per-task runtimes, passes TaskRuntime into executors; tests rewritten to verify acquire/release/error paths with gomock runtime/session helpers.

Import INTO scheduler and executors

Layer / File(s) Summary
Import scheduler and planner integration
pkg/dxf/importinto/scheduler.go
Uses sch.TaskRuntime.Store()/SysSessionPool() for PD client, planning, task registration, and task-manager acquisition.
Import executor and tests
pkg/dxf/importinto/task_executor.go, pkg/dxf/importinto/*_test.go
Step executors use TaskRuntime.Store(); tests updated to inject mocked runtimes and session pools.

dxfutil helper and tests

Layer / File(s) Summary
Runtime keyspace validation helper
pkg/dxf/framework/dxfutil/util.go, pkg/dxf/framework/dxfutil/util_test.go, pkg/dxf/framework/dxfutil/BUILD.bazel
Adds CheckRuntime(runtime, taskKS) to validate store and session pool keyspace consistency and corresponding tests and BUILD entries.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • pingcap/tidb#69013: Cross-keyspace GC/eviction logic that relies on session-pool accessor changes introduced here.
  • pingcap/tidb#68824: Overlaps DDL backfilling store/runtime resolution; both touch backfill execution paths.
  • pingcap/tidb#68890: Earlier runtime/KSRuntimeHandle abstractions that this PR refactors toward a unified Runtime model.

Suggested labels

ok-to-test

Suggested reviewers

  • joechenrh
  • wjhuang2016
  • GMHDBJD

"I hopped through code and tests with glee,
Runtime handles now set sessions free,
Mocks stand ready, stores travel with tasks,
Cross-ks flows tidy — no more hidden masks,
A rabbit cheers the refactor's spree!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.43% 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: acquiring cross-keyspace runtimes at manager boundaries in the DXF and domain packages, which is the core objective of this PR.
Description check ✅ Passed The description includes an issue reference (ref #68883), a detailed problem summary, comprehensive explanation of changes, checkmarks for unit tests, and a release note section, all aligned with the required template.
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 and usage tips.

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🧹 Nitpick comments (2)
pkg/dxf/framework/taskexecutor/task_executor.go (1)

259-265: ⚡ Quick win

Consider adding explicit nil checks for better error messages.

The keyspace validation at line 259 will panic if e.TaskRuntime is nil or if Store() returns nil, rather than producing a clear error message. While the manager is responsible for setting TaskRuntime before calling Init(), an explicit check would provide better diagnostics if that contract is violated.

🛡️ Suggested defensive check
 func (e *BaseTaskExecutor) Init(_ context.Context) error {
+	if e.TaskRuntime == nil {
+		return errors.New("TaskRuntime not initialized by manager")
+	}
 	storeKS := e.TaskRuntime.Store().GetKeyspace()
 	if storeKS != e.GetTaskBase().Keyspace {
🤖 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/dxf/framework/taskexecutor/task_executor.go` around lines 259 - 265, The
current keyspace validation can panic if e.TaskRuntime or e.TaskRuntime.Store()
is nil; add explicit nil checks before calling Store() and before comparing
keyspaces to produce clear errors. In the Init()/keyspace validation block,
check if e.TaskRuntime == nil and return a traced fmt.Errorf describing "nil
TaskRuntime", then check if sr := e.TaskRuntime.Store(); sr == nil and return a
traced fmt.Errorf "nil Store from TaskRuntime"; only after those checks read
sr.GetKeyspace() and compare with e.GetTaskBase().Keyspace to return the
existing "store keyspace mismatch" error. Ensure you reference e.TaskRuntime,
Store(), GetKeyspace(), and GetTaskBase().Keyspace in the updated checks.
pkg/dxf/importinto/task_executor_testkit_test.go (1)

162-162: Clarify the two “import test runtime” helpers to avoid mix-ups

  • newImportTestRuntime is defined in pkg/dxf/importinto/scheduler_testkit_test.go (same package importinto_test), so task_executor_testkit_test.go can call it; passing nil intentionally results in SysSessionPool() returning a nil DestroyableSessionPool.
  • newImportSchedulerTestRuntime is a separate helper in pkg/dxf/importinto/scheduler_test.go for scheduler tests using util.DestroyableSessionPool.
  • Add a short comment (or rename/consolidate) to make the distinction between these helpers explicit.
🤖 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/dxf/importinto/task_executor_testkit_test.go` at line 162, The test is
confusing two similarly named helpers—newImportTestRuntime (used in
task_executor_testkit_test.go and defined in scheduler_testkit_test.go) and
newImportSchedulerTestRuntime (used in scheduler tests and returns a
util.DestroyableSessionPool) —so add a short clarifying comment next to the
param.TaskRuntime = newImportTestRuntime(...) call (or rename one helper)
stating that newImportTestRuntime intentionally returns a runtime whose
SysSessionPool() yields a nil DestroyableSessionPool, whereas
newImportSchedulerTestRuntime returns a runtime with util.DestroyableSessionPool
for scheduler tests; reference the helper names newImportTestRuntime and
newImportSchedulerTestRuntime in the comment.
🤖 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/ddl/backfilling_dist_scheduler.go`:
- Line 179: Replace the unsafe type assertion of store to kv.StorageWithPD with
a checked assertion and handle the failure path: in the function that calls
store.(kv.StorageWithPD) (the backfilling distribution scheduler code path used
for global sort), perform v, ok := store.(kv.StorageWithPD); if !ok return an
error (or propagate an existing error) explaining that PD-backed storage is
required for global sort, otherwise use v; ensure the error case is used
wherever this code runs so it cannot panic at runtime.

In `@pkg/dxf/framework/taskexecutor/manager_test.go`:
- Line 223: Define and use a shared, documented constant for the DXF bookkeeper
ID formats instead of hardcoded fmt.Sprintf strings: add constants like
executorBookkeeperIDFormat = "DXF/executor/%d" and schedulerBookkeeperIDFormat =
"DXF/scheduler/%d" in a package-visible location, then replace
fmt.Sprintf("DXF/executor/%d", task.ID) in
pkg/dxf/framework/taskexecutor/manager.go, fmt.Sprintf("DXF/scheduler/%d",
task.ID) in pkg/dxf/framework/scheduler/scheduler_manager.go, and the hardcoded
literal in pkg/dxf/framework/taskexecutor/manager_test.go (the
server.EXPECT().AcquireKSRuntime call) to use fmt.Sprintf with the new constants
so all code and tests reference the same documented formats.

---

Nitpick comments:
In `@pkg/dxf/framework/taskexecutor/task_executor.go`:
- Around line 259-265: The current keyspace validation can panic if
e.TaskRuntime or e.TaskRuntime.Store() is nil; add explicit nil checks before
calling Store() and before comparing keyspaces to produce clear errors. In the
Init()/keyspace validation block, check if e.TaskRuntime == nil and return a
traced fmt.Errorf describing "nil TaskRuntime", then check if sr :=
e.TaskRuntime.Store(); sr == nil and return a traced fmt.Errorf "nil Store from
TaskRuntime"; only after those checks read sr.GetKeyspace() and compare with
e.GetTaskBase().Keyspace to return the existing "store keyspace mismatch" error.
Ensure you reference e.TaskRuntime, Store(), GetKeyspace(), and
GetTaskBase().Keyspace in the updated checks.

In `@pkg/dxf/importinto/task_executor_testkit_test.go`:
- Line 162: The test is confusing two similarly named
helpers—newImportTestRuntime (used in task_executor_testkit_test.go and defined
in scheduler_testkit_test.go) and newImportSchedulerTestRuntime (used in
scheduler tests and returns a util.DestroyableSessionPool) —so add a short
clarifying comment next to the param.TaskRuntime = newImportTestRuntime(...)
call (or rename one helper) stating that newImportTestRuntime intentionally
returns a runtime whose SysSessionPool() yields a nil DestroyableSessionPool,
whereas newImportSchedulerTestRuntime returns a runtime with
util.DestroyableSessionPool for scheduler tests; reference the helper names
newImportTestRuntime and newImportSchedulerTestRuntime in the comment.
🪄 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

Run ID: 8e26b2e5-cf02-4565-b04b-907f967ffc4e

📥 Commits

Reviewing files that changed from the base of the PR and between 452e517 and efcaa02.

📒 Files selected for processing (32)
  • Makefile
  • pkg/ddl/BUILD.bazel
  • pkg/ddl/backfilling_dist_executor.go
  • pkg/ddl/backfilling_dist_scheduler.go
  • pkg/ddl/backfilling_dist_scheduler_test.go
  • pkg/domain/crossks/cross_ks.go
  • pkg/domain/crossks/cross_ks_internal_test.go
  • pkg/domain/crossks/cross_ks_test.go
  • pkg/domain/domain.go
  • pkg/domain/sqlsvrapi/mock/BUILD.bazel
  • pkg/domain/sqlsvrapi/mock/ksruntime_mock.go
  • pkg/domain/sqlsvrapi/mock/runtime_mock.go
  • pkg/domain/sqlsvrapi/mock/server_mock.go
  • pkg/domain/sqlsvrapi/server.go
  • pkg/dxf/framework/scheduler/BUILD.bazel
  • pkg/dxf/framework/scheduler/interface.go
  • pkg/dxf/framework/scheduler/scheduler.go
  • pkg/dxf/framework/scheduler/scheduler_manager.go
  • pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go
  • pkg/dxf/framework/taskexecutor/BUILD.bazel
  • pkg/dxf/framework/taskexecutor/manager.go
  • pkg/dxf/framework/taskexecutor/manager_test.go
  • pkg/dxf/framework/taskexecutor/task_executor.go
  • pkg/dxf/framework/taskexecutor/task_executor_test.go
  • pkg/dxf/framework/taskexecutor/task_executor_testkit_test.go
  • pkg/dxf/importinto/BUILD.bazel
  • pkg/dxf/importinto/scheduler.go
  • pkg/dxf/importinto/scheduler_test.go
  • pkg/dxf/importinto/scheduler_testkit_test.go
  • pkg/dxf/importinto/task_executor.go
  • pkg/dxf/importinto/task_executor_test.go
  • pkg/dxf/importinto/task_executor_testkit_test.go

return generateGlobalSortIngestPlan(
ctx,
sch.TaskStore.(kv.StorageWithPD),
store.(kv.StorageWithPD),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Verify store type before casting to kv.StorageWithPD.

The unsafe type assertion store.(kv.StorageWithPD) will panic if the store does not implement the StorageWithPD interface. While this is only reached for global sort (which requires PD), consider a checked type assertion with an error return for safer failure handling.

🛡️ Proposed fix to add checked assertion
-		return generateGlobalSortIngestPlan(
-			ctx,
-			store.(kv.StorageWithPD),
-			taskHandle,
-			task,
-			backfillMeta.CloudStorageURI,
-			logger)
+		storePD, ok := store.(kv.StorageWithPD)
+		if !ok {
+			return nil, errors.Errorf("global sort requires StorageWithPD but got %T", store)
+		}
+		return generateGlobalSortIngestPlan(
+			ctx,
+			storePD,
+			taskHandle,
+			task,
+			backfillMeta.CloudStorageURI,
+			logger)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
store.(kv.StorageWithPD),
storePD, ok := store.(kv.StorageWithPD)
if !ok {
return nil, errors.Errorf("global sort requires StorageWithPD but got %T", store)
}
return generateGlobalSortIngestPlan(
ctx,
storePD,
taskHandle,
task,
backfillMeta.CloudStorageURI,
logger)
🤖 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/ddl/backfilling_dist_scheduler.go` at line 179, Replace the unsafe type
assertion of store to kv.StorageWithPD with a checked assertion and handle the
failure path: in the function that calls store.(kv.StorageWithPD) (the
backfilling distribution scheduler code path used for global sort), perform v,
ok := store.(kv.StorageWithPD); if !ok return an error (or propagate an existing
error) explaining that PD-backed storage is required for global sort, otherwise
use v; ensure the error case is used wherever this code runs so it cannot panic
at runtime.

runtimeHandle := newTaskExecutorTestRuntimeHandle(ctrl, taskStore)
runtimeHandle.EXPECT().Release()
server := sqlsvrapimock.NewMockServer(ctrl)
server.EXPECT().AcquireKSRuntime(task.Keyspace, "DXF/executor/201").Return(runtimeHandle, nil)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for bookkeeper ID patterns in manager implementations
rg -n 'DXF/(executor|scheduler)' --type=go --glob='**/scheduler_manager.go' --glob='**/taskexecutor/manager.go' -C2

Repository: pingcap/tidb

Length of output: 982


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Inspect the exact test line context
echo "=== manager_test.go around line 223 ==="
nl -ba pkg/dxf/framework/taskexecutor/manager_test.go | sed -n '200,260p'

# 2) Check if there are existing constants/docs for the bookkeeper naming
echo "=== Search for DXF/executor format constant or comment ==="
rg -n 'DXF/executor/' --type=go --glob='**/*.go'

echo "=== Search for DXF/scheduler format constant or comment ==="
rg -n 'DXF/scheduler/' --type=go --glob='**/*.go'

echo "=== Search for potential existing format const ==="
rg -n 'bookkeeperIDFormat|bookkeeper.*format|Bookkeeper.*ID|DXF/(executor|scheduler).*%d' --type=go --glob='**/*.go'

Repository: pingcap/tidb

Length of output: 157


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== manager_test.go around line 223 ==="
sed -n '210,240p' pkg/dxf/framework/taskexecutor/manager_test.go
echo
echo "=== manager_test.go (with line numbers) around line 223 ==="
cat -n pkg/dxf/framework/taskexecutor/manager_test.go | sed -n '210,240p'

echo
echo "=== Search for DXF/executor/%d occurrences ==="
rg -n 'DXF/executor/%d' --type=go --glob='**/*.go'

echo
echo "=== Search for DXF/scheduler/%d occurrences ==="
rg -n 'DXF/scheduler/%d' --type=go --glob='**/*.go'

echo
echo "=== Search for existing bookkeeper ID format constants/docs ==="
rg -n 'bookkeeper.*(id|ID)|Bookkeeper.*(id|ID)|ID.*format|DXF/(executor|scheduler).*%d' --type=go --glob='**/*.go'

Repository: pingcap/tidb

Length of output: 16861


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== All occurrences of DXF executor/scheduler bookkeeper IDs (string literals) in Go ==="
rg -n 'DXF/(executor|scheduler)/' --type=go --glob='**/*.go'

Repository: pingcap/tidb

Length of output: 1697


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== pkg/dxf/framework/taskexecutor/manager.go around bookkeeper fmt.Sprintf ==="
sed -n '300,380p' pkg/dxf/framework/taskexecutor/manager.go

echo
echo "=== pkg/dxf/framework/scheduler/scheduler_manager.go around bookkeeper fmt.Sprintf ==="
sed -n '330,400p' pkg/dxf/framework/scheduler/scheduler_manager.go

Repository: pingcap/tidb

Length of output: 5088


Document and centralize DXF bookkeeper ID naming formats used for KS runtime acquisition

  • pkg/dxf/framework/taskexecutor/manager.go acquires KS runtime using fmt.Sprintf("DXF/executor/%d", task.ID), and pkg/dxf/framework/taskexecutor/manager_test.go hardcodes "DXF/executor/<taskID>" (e.g., "DXF/executor/201").
  • pkg/dxf/framework/scheduler/scheduler_manager.go acquires KS runtime using fmt.Sprintf("DXF/scheduler/%d", task.ID).

Add a documented constant (or shared helper) and reuse it in both managers and tests to avoid drifting string literals, e.g.:

// Bookkeeper ID format for KS runtime handles.
// Executor-owned: "DXF/executor/<taskID>"
// Scheduler-owned: "DXF/scheduler/<taskID>"
const (
	executorBookkeeperIDFormat  = "DXF/executor/%d"
	schedulerBookkeeperIDFormat = "DXF/scheduler/%d"
)
🤖 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/dxf/framework/taskexecutor/manager_test.go` at line 223, Define and use a
shared, documented constant for the DXF bookkeeper ID formats instead of
hardcoded fmt.Sprintf strings: add constants like executorBookkeeperIDFormat =
"DXF/executor/%d" and schedulerBookkeeperIDFormat = "DXF/scheduler/%d" in a
package-visible location, then replace fmt.Sprintf("DXF/executor/%d", task.ID)
in pkg/dxf/framework/taskexecutor/manager.go, fmt.Sprintf("DXF/scheduler/%d",
task.ID) in pkg/dxf/framework/scheduler/scheduler_manager.go, and the hardcoded
literal in pkg/dxf/framework/taskexecutor/manager_test.go (the
server.EXPECT().AcquireKSRuntime call) to use fmt.Sprintf with the new constants
so all code and tests reference the same documented formats.

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 2.70270% with 108 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.4506%. Comparing base (452e517) to head (7c8e515).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #69023        +/-   ##
================================================
+ Coverage   76.3214%   76.4506%   +0.1291%     
================================================
  Files          2041       2058        +17     
  Lines        562686     576648     +13962     
================================================
+ Hits         429450     440851     +11401     
- Misses       132323     133645      +1322     
- Partials        913       2152      +1239     
Flag Coverage Δ
integration 45.7991% <2.7027%> (+6.0704%) ⬆️

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

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

@ingress-bot

Copy link
Copy Markdown

🔍 Starting code review for this PR...

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
pkg/dxf/framework/scheduler/scheduler_nokit_test.go (1)

42-65: ⚖️ Poor tradeoff

Consider extracting shared test utilities to reduce duplication.

The helpers newSchedulerRuntimeSessionPool and newSchedulerMockRuntime are nearly identical to newCheckRuntimeSessionPool and newCheckRuntimeMockRuntime in pkg/dxf/framework/dxfutil/util_test.go. While package boundaries prevent direct sharing of unexported test helpers, this duplication could make maintenance harder if the mock setup needs to evolve.

♻️ Potential approach

If these helpers are expected to be reused across multiple scheduler or executor test packages, consider creating a shared test utility package (e.g., pkg/dxf/framework/testutil) with exported helper functions. However, given that these are simple wrappers and the duplication is isolated to two test files, keeping them separate is also reasonable.

🤖 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/dxf/framework/scheduler/scheduler_nokit_test.go` around lines 42 - 65,
The two helpers newSchedulerRuntimeSessionPool and newSchedulerMockRuntime
duplicate newCheckRuntimeSessionPool and newCheckRuntimeMockRuntime — extract
the common logic into a shared test helper package with exported functions
(e.g., NewRuntimeSessionPool, NewMockRuntime) and update the tests to call those
exported helpers; ensure the new helpers preserve the same signatures (accepting
*testing.T, kv.Storage, *gomock.Controller, tidbutil.DestroyableSessionPool as
needed) and keep the same behavior (creating utilmock.NewContext, setting Store,
registering t.Cleanup for Close, and setting up mock expectations) so both
scheduler and check tests can reuse them.
🤖 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/dxf/framework/scheduler/scheduler_nokit_test.go`:
- Around line 42-65: The two helpers newSchedulerRuntimeSessionPool and
newSchedulerMockRuntime duplicate newCheckRuntimeSessionPool and
newCheckRuntimeMockRuntime — extract the common logic into a shared test helper
package with exported functions (e.g., NewRuntimeSessionPool, NewMockRuntime)
and update the tests to call those exported helpers; ensure the new helpers
preserve the same signatures (accepting *testing.T, kv.Storage,
*gomock.Controller, tidbutil.DestroyableSessionPool as needed) and keep the same
behavior (creating utilmock.NewContext, setting Store, registering t.Cleanup for
Close, and setting up mock expectations) so both scheduler and check tests can
reuse them.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5dfb9546-da72-4d7d-b653-3d1ec9833b4d

📥 Commits

Reviewing files that changed from the base of the PR and between efcaa02 and f5ffb69.

📒 Files selected for processing (11)
  • pkg/domain/domain.go
  • pkg/domain/sqlsvrapi/server.go
  • pkg/dxf/framework/dxfutil/BUILD.bazel
  • pkg/dxf/framework/dxfutil/util.go
  • pkg/dxf/framework/dxfutil/util_test.go
  • pkg/dxf/framework/scheduler/BUILD.bazel
  • pkg/dxf/framework/scheduler/scheduler.go
  • pkg/dxf/framework/scheduler/scheduler_nokit_test.go
  • pkg/dxf/framework/taskexecutor/BUILD.bazel
  • pkg/dxf/framework/taskexecutor/task_executor.go
  • pkg/dxf/framework/taskexecutor/task_executor_test.go
✅ Files skipped from review due to trivial changes (2)
  • pkg/dxf/framework/dxfutil/BUILD.bazel
  • pkg/dxf/framework/taskexecutor/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (6)
  • pkg/dxf/framework/scheduler/BUILD.bazel
  • pkg/dxf/framework/scheduler/scheduler.go
  • pkg/dxf/framework/taskexecutor/task_executor_test.go
  • pkg/domain/sqlsvrapi/server.go
  • pkg/dxf/framework/taskexecutor/task_executor.go
  • pkg/domain/domain.go

@D3Hunter

D3Hunter commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Jun 9, 2026

Copy link
Copy Markdown

@D3Hunter: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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 kubernetes-sigs/prow repository.

@D3Hunter

D3Hunter commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Jun 9, 2026

Copy link
Copy Markdown

@D3Hunter: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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 kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants