Repository navigation
dxf: acquire cross-keyspace runtimes in framework managers - #69065
Conversation
ti-chi-bot
Bot
added
the
release-note
|
@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
Bot
added
the
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds dxfutil runtime helpers to acquire and validate per-task sqlsvrapi.Runtime, threads TaskRuntime through scheduler and taskexecutor params, updates start paths to acquire/release KS runtimes, and adds tests and BUILD rule updates covering cross-keyspace and mismatch cases. ChangesCross-Keyspace Runtime Integration for DXF Scheduler and TaskExecutor
Sequence Diagram(s)sequenceDiagram
participant Manager
participant SessionProvider
participant SQLServer
participant KSRuntimeHandle
Manager->>SessionProvider: WithNewSession(cb)
SessionProvider->>SQLServer: GetRuntime() or AcquireKSRuntime(taskKS, holder)
SQLServer-->>Manager: Runtime (KSRuntimeHandle if acquired)
Manager->>KSRuntimeHandle: Release() (via returned release closure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dxf/framework/scheduler/scheduler_manager.go (1)
378-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nil scheduler factory before invoking it to avoid panic + runtime-handle leak.
Line 378 can still return nil (e.g., type drift between task fetch phases). Calling it at Line 379 panics, and an acquired cross-keyspace runtime from earlier lines is never released.
Suggested fix
schedulerFactory := getSchedulerFactory(task.Type) + if schedulerFactory == nil { + err := errors.Errorf("unknown task type %s", task.Type) + sm.logger.Warn("unknown task type when starting scheduler", + zap.Int64("task-id", task.ID), + zap.String("task-key", task.Key), + zap.Error(err)) + sm.failTask(task.ID, task.State, err) + releaseRuntime() + return + } scheduler := schedulerFactory(sm.ctx, task, Param{ taskMgr: sm.taskMgr, nodeMgr: sm.nodeMgr, slotMgr: sm.slotMgr,🤖 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_manager.go` around lines 378 - 388, Check whether getSchedulerFactory(task.Type) returned nil before calling it: if schedulerFactory is nil, release the previously acquired task runtime (taskRuntime) using its cleanup method (e.g., Release/Close) to avoid the runtime handle leak, and return or continue with an error path instead of invoking schedulerFactory; otherwise call schedulerFactory as before to create scheduler. Ensure you reference getSchedulerFactory, schedulerFactory, scheduler and taskRuntime/TaskRuntime in the fix.
🧹 Nitpick comments (1)
pkg/dxf/framework/dxfutil/BUILD.bazel (1)
21-21: ⚡ Quick winDrop default flakiness for this new unit test target.
flaky = Trueon Line 21 weakens CI signal and can hide regressions; prefer keeping it non-flaky unless there is a known tracked flake.Suggested change
go_test( name = "dxfutil_test", timeout = "short", srcs = ["util_test.go"], embed = [":dxfutil"], - flaky = True, deps = [🤖 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/dxfutil/BUILD.bazel` at line 21, The test target currently sets flaky = True which weakens CI signal; remove the flaky = True attribute (or set flaky = False) from the target declaration in BUILD.bazel so the unit test runs as a non-flaky target; locate the entry that contains the flaky = True line and delete or change that attribute to False in the target block.
🤖 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/dxf/framework/taskexecutor/task_executor.go`:
- Around line 86-95: NewParamForTest builds a Param but leaves Param.TaskRuntime
nil, which causes BaseTaskExecutor.Init to panic when it calls
dxfutil.CheckRuntime(e.TaskRuntime,...). Update NewParamForTest to initialize
TaskRuntime using the provided TaskStore (kv.Storage) — e.g., create/obtain the
runtime instance from the store via your existing runtime constructor/factory
and assign it to param.TaskRuntime — or explicitly document that callers must
set Param.TaskRuntime before calling Init; reference NewParamForTest,
Param.TaskRuntime, TaskStore, BaseTaskExecutor.Init, and dxfutil.CheckRuntime
when making the change.
---
Outside diff comments:
In `@pkg/dxf/framework/scheduler/scheduler_manager.go`:
- Around line 378-388: Check whether getSchedulerFactory(task.Type) returned nil
before calling it: if schedulerFactory is nil, release the previously acquired
task runtime (taskRuntime) using its cleanup method (e.g., Release/Close) to
avoid the runtime handle leak, and return or continue with an error path instead
of invoking schedulerFactory; otherwise call schedulerFactory as before to
create scheduler. Ensure you reference getSchedulerFactory, schedulerFactory,
scheduler and taskRuntime/TaskRuntime in the fix.
---
Nitpick comments:
In `@pkg/dxf/framework/dxfutil/BUILD.bazel`:
- Line 21: The test target currently sets flaky = True which weakens CI signal;
remove the flaky = True attribute (or set flaky = False) from the target
declaration in BUILD.bazel so the unit test runs as a non-flaky target; locate
the entry that contains the flaky = True line and delete or change that
attribute to False in the target block.
🪄 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: 3c8e6a54-d615-41f1-bf13-0525a967644f
📒 Files selected for processing (14)
pkg/dxf/framework/dxfutil/BUILD.bazelpkg/dxf/framework/dxfutil/util.gopkg/dxf/framework/dxfutil/util_test.gopkg/dxf/framework/scheduler/BUILD.bazelpkg/dxf/framework/scheduler/interface.gopkg/dxf/framework/scheduler/scheduler.gopkg/dxf/framework/scheduler/scheduler_manager.gopkg/dxf/framework/scheduler/scheduler_manager_nokit_test.gopkg/dxf/framework/scheduler/scheduler_nokit_test.gopkg/dxf/framework/taskexecutor/BUILD.bazelpkg/dxf/framework/taskexecutor/manager.gopkg/dxf/framework/taskexecutor/manager_test.gopkg/dxf/framework/taskexecutor/task_executor.gopkg/dxf/framework/taskexecutor/task_executor_test.go
| func NewParamForTest(taskTable TaskTable, slotMgr *slotManager, nodeRc *proto.NodeResource, execID string, store kv.Storage) Param { | ||
| return Param{ | ||
| param := Param{ | ||
| taskTable: taskTable, | ||
| slotMgr: slotMgr, | ||
| nodeRc: nodeRc, | ||
| execID: execID, | ||
| TaskStore: store, | ||
| } | ||
| return param | ||
| } |
There was a problem hiding this comment.
NewParamForTest does not initialize TaskRuntime, which may cause nil-pointer issues.
The function creates a Param without setting TaskRuntime. Since BaseTaskExecutor.Init now calls dxfutil.CheckRuntime(e.TaskRuntime, ...), tests using NewParamForTest followed by Init will panic on a nil TaskRuntime.
Consider initializing TaskRuntime from the provided store, or document that callers must set it separately.
Suggested fix
func NewParamForTest(taskTable TaskTable, slotMgr *slotManager, nodeRc *proto.NodeResource, execID string, store kv.Storage) Param {
param := Param{
taskTable: taskTable,
slotMgr: slotMgr,
nodeRc: nodeRc,
execID: execID,
TaskStore: store,
+ // Note: Callers needing Init() must set TaskRuntime explicitly.
}
return param
}📝 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.
| func NewParamForTest(taskTable TaskTable, slotMgr *slotManager, nodeRc *proto.NodeResource, execID string, store kv.Storage) Param { | |
| return Param{ | |
| param := Param{ | |
| taskTable: taskTable, | |
| slotMgr: slotMgr, | |
| nodeRc: nodeRc, | |
| execID: execID, | |
| TaskStore: store, | |
| } | |
| return param | |
| } | |
| func NewParamForTest(taskTable TaskTable, slotMgr *slotManager, nodeRc *proto.NodeResource, execID string, store kv.Storage) Param { | |
| param := Param{ | |
| taskTable: taskTable, | |
| slotMgr: slotMgr, | |
| nodeRc: nodeRc, | |
| execID: execID, | |
| TaskStore: store, | |
| // Note: Callers needing Init() must set TaskRuntime explicitly. | |
| } | |
| return param | |
| } |
🤖 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 86 - 95,
NewParamForTest builds a Param but leaves Param.TaskRuntime nil, which causes
BaseTaskExecutor.Init to panic when it calls
dxfutil.CheckRuntime(e.TaskRuntime,...). Update NewParamForTest to initialize
TaskRuntime using the provided TaskStore (kv.Storage) — e.g., create/obtain the
runtime instance from the store via your existing runtime constructor/factory
and assign it to param.TaskRuntime — or explicitly document that callers must
set Param.TaskRuntime before calling Init; reference NewParamForTest,
Param.TaskRuntime, TaskStore, BaseTaskExecutor.Init, and dxfutil.CheckRuntime
when making the change.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69065 +/- ##
================================================
- Coverage 76.3233% 74.8955% -1.4278%
================================================
Files 2041 2030 -11
Lines 562646 571250 +8604
================================================
- Hits 429430 427841 -1589
- Misses 132305 143216 +10911
+ Partials 911 193 -718
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dxf/framework/scheduler/interface.go (1)
193-198: Review: Guard against nilTaskRuntimeonly whenInit()is called
scheduler.NewParamForTestleavesParam.TaskRuntimenil, andBaseScheduler.Init()callsdxfutil.CheckRuntime(s.TaskRuntime, ...), which unconditionally invokesruntime.Store()—so callingInit()with thisParamwould panic.Current usages of
scheduler.NewParamForTestare limited topkg/dxf/importinto/scheduler_testkit_test.go(4 call sites), and that test file does not callInit()on the returned scheduler, so the panic path isn’t exercised. If future tests callInit()for schedulers created via this helper, they must setTaskRuntime(mock runtime) or avoidInit().🤖 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/interface.go` around lines 193 - 198, NewParamForTest currently leaves Param.TaskRuntime nil which will cause BaseScheduler.Init to panic because BaseScheduler.Init calls dxfutil.CheckRuntime(s.TaskRuntime, ...) and that ends up invoking runtime.Store(); fix by initializing Param.TaskRuntime in NewParamForTest to a lightweight/noop/mock TaskRuntime instance so callers that later call BaseScheduler.Init won't hit the nil path (i.e., set Param.TaskRuntime to a simple mock/noop runtime when constructing the Param in NewParamForTest); alternatively, if you prefer the nil-safe approach, add a nil-check in BaseScheduler.Init before calling dxfutil.CheckRuntime/Store to skip runtime.Store when TaskRuntime is nil (reference symbols: NewParamForTest, Param.TaskRuntime, BaseScheduler.Init, dxfutil.CheckRuntime, runtime.Store).
🤖 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/interface.go`:
- Around line 193-198: NewParamForTest currently leaves Param.TaskRuntime nil
which will cause BaseScheduler.Init to panic because BaseScheduler.Init calls
dxfutil.CheckRuntime(s.TaskRuntime, ...) and that ends up invoking
runtime.Store(); fix by initializing Param.TaskRuntime in NewParamForTest to a
lightweight/noop/mock TaskRuntime instance so callers that later call
BaseScheduler.Init won't hit the nil path (i.e., set Param.TaskRuntime to a
simple mock/noop runtime when constructing the Param in NewParamForTest);
alternatively, if you prefer the nil-safe approach, add a nil-check in
BaseScheduler.Init before calling dxfutil.CheckRuntime/Store to skip
runtime.Store when TaskRuntime is nil (reference symbols: NewParamForTest,
Param.TaskRuntime, BaseScheduler.Init, dxfutil.CheckRuntime, runtime.Store).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb417f69-77a5-443f-98eb-83bf166b247d
📒 Files selected for processing (3)
pkg/dxf/framework/scheduler/interface.gopkg/dxf/framework/scheduler/scheduler_manager_nokit_test.gopkg/dxf/framework/scheduler/scheduler_nokit_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/dxf/framework/scheduler/scheduler_manager_nokit_test.go
- pkg/dxf/framework/scheduler/scheduler_nokit_test.go
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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 kubernetes-sigs/prow repository. |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 7
- Inline comments: 7
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (2)
- Base Init no longer honors TaskStore-only params (
pkg/dxf/framework/scheduler/scheduler.go:126; pkg/dxf/framework/taskexecutor/task_executor.go:264; pkg/dxf/framework/scheduler/interface.go:193; pkg/dxf/framework/taskexecutor/task_executor.go:86) - Scheduler runtime handle is not released if factory lookup fails (
pkg/dxf/framework/scheduler/scheduler_manager.go:356)
🟡 [Minor] (4)
- Runtime validation name drops the task-keyspace contract (
pkg/dxf/framework/dxfutil/util.go:63) - Runtime holder identifier uses two names (
pkg/dxf/framework/scheduler/scheduler_manager.go:355) - Runtime validation utility is coupled to task storage internals (
pkg/dxf/framework/dxfutil/util.go:63) - New dxfutil test target is marked flaky (
pkg/dxf/framework/dxfutil/BUILD.bazel:21)
ℹ️ [Info] (1)
- SessionProvider adds an unnecessary exported helper type (
pkg/dxf/framework/dxfutil/util.go:26)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dxf/framework/dxfutil/util.go (1)
64-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
CheckTaskRuntimeagainst nil runtime internals to avoid panic paths.At Line 65,
runtime.Store().GetKeyspace()can panic ifruntime(orStore()) is nil; similarlyruntime.SysSessionPool()at Line 72 can be nil. This validator should return contextual errors instead of crashing caller initialization.Suggested patch
func CheckTaskRuntime(runtime sqlsvrapi.Runtime, taskKS string) error { - storeKS := runtime.Store().GetKeyspace() + if runtime == nil { + return errors.Trace(fmt.Errorf("invalid task runtime: runtime is nil")) + } + store := runtime.Store() + if store == nil { + return errors.Trace(fmt.Errorf("invalid task runtime: store is nil")) + } + if runtime.SysSessionPool() == nil { + return errors.Trace(fmt.Errorf("invalid task runtime: sys session pool is nil")) + } + storeKS := store.GetKeyspace() if storeKS != taskKS { // shouldn't happen normally, but since keyspace mismatch might cause // correctness error, we check it at runtime too. return errors.Trace(fmt.Errorf("store keyspace mismatch with task: %s vs %s", storeKS, taskKS)) }🤖 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/dxfutil/util.go` around lines 64 - 75, In CheckTaskRuntime, guard against nil internals: check that runtime is non-nil, that runtime.Store() is non-nil before calling GetKeyspace(), and that runtime.SysSessionPool() is non-nil before passing it to storage.NewTaskManager; if any are nil return a contextual error (use errors.Trace(fmt.Errorf(...))) instead of proceeding. Also inside the taskMgr.WithNewSession callback, validate that the session context (se) and se.GetStore() are non-nil before calling GetKeyspace() and return a descriptive error if they are; reference CheckTaskRuntime, runtime.Store(), runtime.SysSessionPool(), storage.NewTaskManager, taskMgr.WithNewSession, and se.GetStore() when locating where to add these checks.
🤖 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.
Outside diff comments:
In `@pkg/dxf/framework/dxfutil/util.go`:
- Around line 64-75: In CheckTaskRuntime, guard against nil internals: check
that runtime is non-nil, that runtime.Store() is non-nil before calling
GetKeyspace(), and that runtime.SysSessionPool() is non-nil before passing it to
storage.NewTaskManager; if any are nil return a contextual error (use
errors.Trace(fmt.Errorf(...))) instead of proceeding. Also inside the
taskMgr.WithNewSession callback, validate that the session context (se) and
se.GetStore() are non-nil before calling GetKeyspace() and return a descriptive
error if they are; reference CheckTaskRuntime, runtime.Store(),
runtime.SysSessionPool(), storage.NewTaskManager, taskMgr.WithNewSession, and
se.GetStore() when locating where to add these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b58c938f-a98b-4ea6-a854-11eb65632fd5
📒 Files selected for processing (8)
pkg/dxf/framework/dxfutil/util.gopkg/dxf/framework/dxfutil/util_test.gopkg/dxf/framework/scheduler/scheduler.gopkg/dxf/framework/scheduler/scheduler_manager.gopkg/dxf/framework/scheduler/scheduler_manager_nokit_test.gopkg/dxf/framework/taskexecutor/manager.gopkg/dxf/framework/taskexecutor/manager_test.gopkg/dxf/framework/taskexecutor/task_executor.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/dxf/framework/scheduler/scheduler_manager.go
- pkg/dxf/framework/taskexecutor/task_executor.go
- pkg/dxf/framework/dxfutil/util_test.go
- pkg/dxf/framework/taskexecutor/manager_test.go
|
🔍 Starting code review for this PR... |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package dxfutil |
There was a problem hiding this comment.
Why not naming util directly, since it lies in dxf package
There was a problem hiding this comment.
as there are many pkg called util, if we import them both, we have to add alias(such as dxfutil) to difference them, as we have done for this one in taskexecutor pkg. so i choose to name it as dxfutil directly
tidbutil "github.com/pingcap/tidb/pkg/util"
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: 6
- Inline comments: 3
- Summary-only findings (no inline anchor): 3
Findings (highest risk first)
🟡 [Minor] (3)
- Transitional
TaskStorecomment lacks removal tracking on a public field (pkg/dxf/framework/scheduler/interface.go:188, pkg/dxf/framework/taskexecutor/task_executor.go:81) - New mock-runtime / session-pool test helpers duplicated across four DXF test packages (pkg/dxf/framework/scheduler/scheduler_nokit_test.go:54, pkg/dxf/framework/taskexecutor/task_executor_test.go:56, pkg/dxf/importinto/scheduler_test.go:48, pkg/dxf/framework/dxfutil/util_test.go:188)
- Executor releases the keyspace runtime handle after removing itself from the started-set, opening a holderID-collision restart window (pkg/dxf/framework/taskexecutor/manager.go:370, pkg/dxf/framework/scheduler/scheduler_manager.go:386, pkg/domain/crossks/cross_ks.go:145)
ℹ️ [Info] (1)
- Runtime handle release is not panic-safe, unlike the adjacent deferred slot free (pkg/dxf/framework/taskexecutor/manager.go:335, pkg/dxf/framework/scheduler/scheduler_manager.go:356, pkg/domain/crossks/cross_ks.go:383)
🧹 [Nit] (2)
- Init keyspace-check tests bundle three scenarios into one function, hurting failure isolation (pkg/dxf/framework/scheduler/scheduler_nokit_test.go:80, pkg/dxf/framework/taskexecutor/task_executor_test.go:173)
- Unrelated log-line reformatting mixed into the runtime-migration change (pkg/dxf/framework/scheduler/scheduler.go:119)
Unanchored findings
🟡 [Minor] (1)
- Transitional
TaskStorecomment lacks removal tracking on a public field- Request: Add a tracking reference (issue/PR) and concrete removal condition to the
TaskStorecomment, or mark itDeprecated:so callers know it is the legacy field and which migration unblocks its removal. Keep the wording identical across bothParamstructs.
- Request: Add a tracking reference (issue/PR) and concrete removal condition to the
ℹ️ [Info] (1)
- Runtime handle release is not panic-safe, unlike the adjacent deferred slot free
- Request: Acquire the handle under a deferred release with an ownership-transfer flag (release only when not handed to the running goroutine), so a panic during factory/
Initcannot leak the keyspace runtime, consistent with how the slot is already protected.
- Request: Acquire the handle under a deferred release with an ownership-transfer flag (release only when not handed to the running goroutine), so a panic during factory/
🧹 [Nit] (1)
- Init keyspace-check tests bundle three scenarios into one function, hurting failure isolation
- Request: Wrap each keyspace scenario in its own
t.Runsubtest (as inutil_test.go) so a single regressing branch fails in isolation and remains diagnosable.
- Request: Wrap each keyspace scenario in its own
| return sePool | ||
| } | ||
|
|
||
| func newMockRuntime( |
There was a problem hiding this comment.
🟡 [Minor] New mock-runtime / session-pool test helpers duplicated across four DXF test packages
Impact
The same newMockRuntime and newSessionPoolForStore scaffolding is copy-pasted into four DXF test files instead of living in the existing shared framework/testutil.
Future changes to the sqlsvrapi.Runtime mock surface must be applied in four places, and the copies can silently drift.
Scope
pkg/dxf/framework/scheduler/scheduler_nokit_test.go:54—newMockRuntimepkg/dxf/framework/taskexecutor/task_executor_test.go:56—newMockRuntimepkg/dxf/importinto/scheduler_test.go:48—newMockRuntimepkg/dxf/framework/dxfutil/util_test.go:188—newCheckTaskRuntimeMockRuntime
Evidence
newMockRuntime (wrapping sqlsvrapimock.NewMockRuntime + Store()/SysSessionPool() expectations) and newSessionPoolForStore (wrapping tidbutil.NewSessionPool) are defined nearly verbatim in scheduler, taskexecutor, and importinto test files, with the same pattern reimplemented under different names in dxfutil/util_test.go. The PR already imports //pkg/dxf/framework/testutil in these test targets.
Change request
Hoist the shared newMockRuntime / newSessionPoolForStore builders into pkg/dxf/framework/testutil (exported) and consume them from the four test packages to keep one source of truth for the runtime mock setup.
| } | ||
| s.task.Store(task) | ||
| logger.Info("create base scheduler", zap.Stringer("task-type", task.Type), zap.Bool("allocated-slots", param.allocatedSlots)) | ||
| logger.Info("create base scheduler", zap.Stringer("task-type", task.Type), |
There was a problem hiding this comment.
🧹 [Nit] Unrelated log-line reformatting mixed into the runtime-migration change
NewBaseScheduler has an unrelated cosmetic reformat of the existing create base scheduler log statement folded into a behavior-changing PR.
This adds reviewer noise and blurs which lines are functional changes versus formatting churn.
|
[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:
|
What problem does this PR solve?
Issue Number: ref #68883
Problem Summary:
NextGen DXF tasks can execute against user keyspaces while the framework manager runs from the SYSTEM keyspace. The framework previously resolved raw keyspace stores for scheduler and task executor creation, which left runtime ownership implicit and made it harder for cross-keyspace runtime cleanup to know when DXF was still using a runtime.
What changed and how does it work?
pkg/dxf/framework/dxfutil.CheckRuntimeto validate that a task runtime's store and system session pool both point at the task keyspace.sqlsvrapi.Runtimethrough scheduler and task executor params, while keepingTaskStoretemporarily for existing task implementations.DXF/scheduler/<taskID>, and release them after scheduler exit or scheduler initialization failure.DXF/executor/<taskID>, and release them after executor exit or executor startup failure paths.Check List
Tests
Unit tests and manual checks:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit