dxf, server: batch history transfer and bound task cleanup#69811
Open
D3Hunter wants to merge 6 commits into
Open
dxf, server: batch history transfer and bound task cleanup#69811D3Hunter wants to merge 6 commits into
D3Hunter wants to merge 6 commits into
Conversation
dxf: bound task history transfer batches docs: design DXF cleanup batch tuning dxf: add task cleanup batch setting dxf: decouple cleanup batch from concurrency server: expose DXF cleanup batch setting build: update DXF test metadata docs: document DXF cleanup batch API change
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughAdds owner-local DXF cleanup batch tuning, bounded task selection, bulk task and subtask history transfer, and a SYSTEM keyspace HTTP API with tests and documentation. ChangesDXF cleanup batching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DXFTaskCleanupBatchSizeHandler
participant TaskCleanupBatchSize
Client->>DXFTaskCleanupBatchSizeHandler: GET or POST cleanup batch size
DXFTaskCleanupBatchSizeHandler->>TaskCleanupBatchSize: read or validate and update
TaskCleanupBatchSize-->>DXFTaskCleanupBatchSizeHandler: return current value
DXFTaskCleanupBatchSizeHandler-->>Client: return JSON response
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 |
![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
🧹 Nitpick comments (1)
pkg/dxf/framework/storage/table_test.go (1)
1374-1377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert batching for the global-task transfer statements.
The test counts only the metadata update and subtask operations. Also assert the task-history insert and active-task delete occur once, preventing a regression to per-task SQL.
Proposed assertions
require.Equal(t, 1, recorder.countContains("update mysql.tidb_global_task")) recorder.requireContains(t, "set meta = case id") +require.Equal(t, 1, recorder.countContains("insert into mysql.tidb_global_task_history")) +require.Equal(t, 1, recorder.countContains("delete from mysql.tidb_global_task")) require.Equal(t, 1, recorder.countContains("insert into mysql.tidb_background_subtask_history")) require.Equal(t, 1, recorder.countContains("delete from mysql.tidb_background_subtask"))Based on the PR objective to batch both task and subtask history transfers.
🤖 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/storage/table_test.go` around lines 1374 - 1377, Extend the assertions in the relevant transfer test to verify the task-history insert and active-task delete statements each occur exactly once, using the recorder’s count helper and matching the existing global-task SQL patterns. Keep the current metadata and subtask batching assertions unchanged.
🤖 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/scheduler/scheduler_manager.go`:
- Around line 429-456: Update the cleanup loop around cleanupFinishedTasks so
“cleanup routine success” is logged only when allCleaned is true; retain the
existing failure handling and termination behavior for false results.
---
Nitpick comments:
In `@pkg/dxf/framework/storage/table_test.go`:
- Around line 1374-1377: Extend the assertions in the relevant transfer test to
verify the task-history insert and active-task delete statements each occur
exactly once, using the recorder’s count helper and matching the existing
global-task SQL patterns. Keep the current metadata and subtask batching
assertions unchanged.
🪄 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: 2521aa34-7cda-40f6-915a-d9d69c849483
📒 Files selected for processing (15)
.agents/skills/tidb-test-guidelines/references/dxf-case-map.mddocs/tidb_http_api.mdpkg/dxf/framework/proto/BUILD.bazelpkg/dxf/framework/proto/task.gopkg/dxf/framework/proto/task_test.gopkg/dxf/framework/scheduler/scheduler_manager.gopkg/dxf/framework/scheduler/scheduler_manager_nokit_test.gopkg/dxf/framework/storage/BUILD.bazelpkg/dxf/framework/storage/history.gopkg/dxf/framework/storage/table_test.gopkg/dxf/framework/storage/task_table.gopkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/dxf_test.gopkg/server/handler/tikvhandler/dxf.gopkg/server/http_status.go
Comment on lines
429
to
456
| for { | ||
| cleanupBatchSize := proto.GetTaskCleanupBatchSize() | ||
| tasks, err := sm.taskMgr.GetTasksInStates( | ||
| sm.ctx, | ||
| proto.TaskStateFailed, | ||
| proto.TaskStateReverted, | ||
| proto.TaskStateSucceed, | ||
| ) | ||
| if err != nil { | ||
| sm.logger.Warn("get task in states failed", zap.Error(err)) | ||
| return | ||
| } | ||
| if len(tasks) == 0 { | ||
| return | ||
| } | ||
| sm.logger.Info("cleanup routine start") | ||
| allCleaned, err := sm.cleanupFinishedTasks(tasks) | ||
| if err != nil { | ||
| sm.logger.Warn("cleanup routine failed", zap.Error(err)) | ||
| return | ||
| } | ||
| failpoint.InjectCall("WaitCleanUpFinished") | ||
| sm.logger.Info("cleanup routine success") | ||
| if !allCleaned || len(tasks) < cleanupBatchSize { | ||
| return | ||
| } | ||
| } | ||
| failpoint.InjectCall("WaitCleanUpFinished") | ||
| sm.logger.Info("cleanup routine success") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log "cleanup routine success" is emitted even when allCleaned is false.
When a cleanup error occurs (firstErr != nil) but TransferTasks2History succeeds (err == nil), cleanupFinishedTasks returns (false, nil). The doCleanupTask error check passes, and line 451 logs "cleanup routine success" — contradicting the "cleanup routine failed" Warn message already logged inside cleanupFinishedTasks at line 482. This produces contradictory log lines for the same batch.
🔧 Suggested fix: gate the success log on `allCleaned`
failpoint.InjectCall("WaitCleanUpFinished")
- sm.logger.Info("cleanup routine success")
if !allCleaned || len(tasks) < cleanupBatchSize {
+ if !allCleaned {
+ sm.logger.Info("cleanup routine partially failed")
+ }
return
}
+ sm.logger.Info("cleanup routine success")📝 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
| for { | |
| cleanupBatchSize := proto.GetTaskCleanupBatchSize() | |
| tasks, err := sm.taskMgr.GetTasksInStates( | |
| sm.ctx, | |
| proto.TaskStateFailed, | |
| proto.TaskStateReverted, | |
| proto.TaskStateSucceed, | |
| ) | |
| if err != nil { | |
| sm.logger.Warn("get task in states failed", zap.Error(err)) | |
| return | |
| } | |
| if len(tasks) == 0 { | |
| return | |
| } | |
| sm.logger.Info("cleanup routine start") | |
| allCleaned, err := sm.cleanupFinishedTasks(tasks) | |
| if err != nil { | |
| sm.logger.Warn("cleanup routine failed", zap.Error(err)) | |
| return | |
| } | |
| failpoint.InjectCall("WaitCleanUpFinished") | |
| sm.logger.Info("cleanup routine success") | |
| if !allCleaned || len(tasks) < cleanupBatchSize { | |
| return | |
| } | |
| } | |
| failpoint.InjectCall("WaitCleanUpFinished") | |
| sm.logger.Info("cleanup routine success") | |
| } | |
| for { | |
| cleanupBatchSize := proto.GetTaskCleanupBatchSize() | |
| tasks, err := sm.taskMgr.GetTasksInStates( | |
| sm.ctx, | |
| proto.TaskStateFailed, | |
| proto.TaskStateReverted, | |
| proto.TaskStateSucceed, | |
| ) | |
| if err != nil { | |
| sm.logger.Warn("get task in states failed", zap.Error(err)) | |
| return | |
| } | |
| if len(tasks) == 0 { | |
| return | |
| } | |
| sm.logger.Info("cleanup routine start") | |
| allCleaned, err := sm.cleanupFinishedTasks(tasks) | |
| if err != nil { | |
| sm.logger.Warn("cleanup routine failed", zap.Error(err)) | |
| return | |
| } | |
| failpoint.InjectCall("WaitCleanUpFinished") | |
| if !allCleaned || len(tasks) < cleanupBatchSize { | |
| if !allCleaned { | |
| sm.logger.Info("cleanup routine partially failed") | |
| } | |
| return | |
| } | |
| sm.logger.Info("cleanup routine success") | |
| } | |
| } |
🤖 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 429 - 456,
Update the cleanup loop around cleanupFinishedTasks so “cleanup routine success”
is logged only when allCleaned is true; retain the existing failure handling and
termination behavior for false results.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69811 +/- ##
================================================
- Coverage 76.3196% 74.2264% -2.0932%
================================================
Files 2041 2054 +13
Lines 560096 576766 +16670
================================================
+ Hits 427463 428113 +650
- Misses 131732 148302 +16570
+ Partials 901 351 -550
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... |
ti-chi-bot
Bot
added
size/XL
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/storage/task_table.go (1)
438-453: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemoving
ORDER BYcontradicts the PR objective of prioritizing oldest terminal-state transitions.The PR summary states cleanup "prioritizes the oldest terminal-state transitions," but this change removes the
ORDER BYclause entirely. Without explicit ordering, TiDB may return rows in any order — there is no guarantee that the oldest terminal-state tasks are selected first. The downstream consumer (doCleanupTaskinscheduler_manager.go) processes whateverGetTasksInStatesreturns without additional sorting, so the non-determinism propagates directly to cleanup ordering.If oldest-first processing is intended, restore an
ORDER BYclause (e.g.,ORDER BY state_update_time ASC, create_time ASC, id ASC) before theLIMIT. If ordering is no longer a requirement, update the PR summary to avoid misleading documentation.🔧 Proposed fix: restore ORDER BY before LIMIT
rs, err := mgr.ExecuteSQLWithNewSession(ctx, "select "+TaskColumns+" from mysql.tidb_global_task t "+ "where state in ("+strings.Repeat("%?,", len(states)-1)+"%?)"+ + " order by state_update_time, create_time, id"+ " limit %?", args...)🤖 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/storage/task_table.go` around lines 438 - 453, Restore deterministic oldest-first ordering in TaskManager.GetTasksInStates by adding ORDER BY state_update_time ASC, create_time ASC, and id ASC before the existing LIMIT clause, preserving the current state filtering and cleanup batch size behavior.
🤖 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/storage/task_table.go`:
- Around line 438-453: Restore deterministic oldest-first ordering in
TaskManager.GetTasksInStates by adding ORDER BY state_update_time ASC,
create_time ASC, and id ASC before the existing LIMIT clause, preserving the
current state filtering and cleanup batch size behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be39b51d-38fa-44dd-8c49-3198c372d3c7
📒 Files selected for processing (3)
.agents/skills/tidb-test-guidelines/references/dxf-case-map.mdpkg/dxf/framework/storage/table_test.gopkg/dxf/framework/storage/task_table.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/tidb-test-guidelines/references/dxf-case-map.md
ingress-bot
reviewed
Jul 13, 2026
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: 12
- Inline comments: 6
- Summary-only findings (no inline anchor): 4
Findings (highest risk first)
⚠️ [Major] (1)
- Shared multi-consumer interface method GetTasksInStates now hard-codes a cleanup-only limit (pkg/dxf/framework/storage/task_table.go:438, pkg/dxf/framework/scheduler/interface.go:43, pkg/dxf/framework/taskexecutor/interface.go:32)
🟡 [Minor] (11)
- GetTasksInStates doc comment hides both a dropped ordering guarantee and a new silent cap (pkg/dxf/framework/storage/task_table.go:438, pkg/dxf/framework/storage/task_table.go:464, pkg/dxf/framework/scheduler/interface.go:43, pkg/dxf/framework/taskexecutor/interface.go:32)
- DXFTaskCleanupBatchSizeHandler duplicates DXFTaskMaxConcurrentHandler's ServeHTTP structure verbatim (pkg/server/handler/tikvhandler/dxf.go:390, pkg/server/handler/tikvhandler/dxf.go:348)
- Subtask-history transfer logic duplicated instead of reusing TransferSubtasks2HistoryWithSession (pkg/dxf/framework/storage/history.go:98, pkg/dxf/framework/storage/history.go:42)
- Task cleanup batch size knob duplicates the max-concurrent-task knob machinery in proto/task.go verbatim (pkg/dxf/framework/proto/task.go:116, pkg/dxf/framework/proto/task.go:124)
- New task_cleanup_batch_size doc states a SYSTEM-keyspace restriction the sibling max_concurrent_task section omits (docs/tidb_http_api.md:1024, docs/tidb_http_api.md:1058, pkg/server/http_status.go:252)
- New cleanup-batch-size subtest sets an unrelated MaxConcurrentTask override (pkg/dxf/framework/storage/table_test.go:1057)
- TestTaskHistoryTable positionally compares now-unordered GetTasksInStates against ordered GetTaskBasesInStates (pkg/dxf/framework/storage/table_test.go:1085, pkg/dxf/framework/storage/task_table.go:438, pkg/dxf/framework/storage/task_table.go:464)
- No regression test that the batch-limited cleanup still drains all finished tasks across cycles (pkg/dxf/framework/storage/task_table.go:449, pkg/dxf/framework/scheduler/scheduler_manager.go:427, pkg/dxf/framework/storage/table_test.go:1056)
- Cleanup pass is now capped at batchSize with no re-trigger, so a signal-less backlog drains only at the 10-minute ticker (pkg/dxf/framework/storage/task_table.go:453, pkg/dxf/framework/scheduler/scheduler_manager.go:427, pkg/dxf/framework/scheduler/scheduler_manager.go:406)
- GetTasksInStates now silently truncates and unorders results via a cleanup-specific knob (pkg/dxf/framework/storage/task_table.go:453, pkg/dxf/framework/scheduler/interface.go:43, pkg/dxf/framework/taskexecutor/interface.go:32)
- TestTaskHistoryTable couples now-unordered GetTasksInStates result to ordered GetTaskBasesInStates (pkg/dxf/framework/storage/task_table.go:453, pkg/dxf/framework/storage/table_test.go:1085)
Unanchored findings
⚠️ [Major] (1)
- Shared multi-consumer interface method GetTasksInStates now hard-codes a cleanup-only limit
- Request: Scope the cleanup batch limit to the cleanup call site instead of the shared interface method, e.g. add an explicit
limit intparameter (or a dedicated method used only bydoCleanupTask) rather than folding a single caller's tuning knob into a method shared by two independent interfaces. If the shared method must carry the limit, document the capped/unordered behavior on the interface declaration the wayGetTopUnfinishedTasksdoes.
- Request: Scope the cleanup batch limit to the cleanup call site instead of the shared interface method, e.g. add an explicit
🟡 [Minor] (3)
- DXFTaskCleanupBatchSizeHandler duplicates DXFTaskMaxConcurrentHandler's ServeHTTP structure verbatim
- Request: Extract a single generic handler (or a shared
serveBoundedIntKnob(w, req, name string, get func() int, set func(int) error)helper) that bothDXFTaskMaxConcurrentHandlerandDXFTaskCleanupBatchSizeHandlercall, so the GET/POST/parse/log/response logic exists once.
- Request: Extract a single generic handler (or a shared
- Subtask-history transfer logic duplicated instead of reusing TransferSubtasks2HistoryWithSession
- Request: Consolidate: either give
TransferSubtasks2HistoryWithSessiona batched (taskIDs []int64) variant thatTransferTasks2Historycalls, or fold the single-task test helper into the batched form so there is one source of truth for the subtask-history-transfer SQL.
- Request: Consolidate: either give
- Task cleanup batch size knob duplicates the max-concurrent-task knob machinery in proto/task.go verbatim
- Request: Factor the bounded, in-memory, owner-local
atomic.Int64knob pattern (bounds check + load + store + test-restore closure) into one small generic helper inproto(for example aboundedKnobtype withGet/Set/SetForTest), and construct bothmaxConcurrentTaskandtaskCleanupBatchSizefrom it.
- Request: Factor the bounded, in-memory, owner-local
ti-chi-bot
Bot
added
size/XXL
Contributor
Author
|
/cherry-pick release-nextgen-202603 |
Member
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/dxf/framework/storage/table_test.go (1)
1164-1165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe in-memory error assignment is not persisted by
TransferTasks2History.
TransferTasks2Historyonly updates themetacolumn from the provided task slice before copying the row to history; it does not write theErrorfield to the database. SettinguntouchedTask.Error = ...here is an in-memory no-op for the subsequent transfer. If the goal is to test transferring a task with an error, the error must be persisted first (e.g., viaFailTask, as is done correctly in the pagination subtest below).♻️ Proposed fix
// task with fail transfer - untouchedTask.Error = errors.New("mock err") + require.NoError(t, gm.FailTask(ctx, untouchedTask.ID, proto.TaskStatePending, errors.New("mock err"))) require.NoError(t, gm.TransferTasks2History(ctx, []*proto.Task{untouchedTask}))🤖 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/storage/table_test.go` around lines 1164 - 1165, Update the test around TransferTasks2History so the task error is persisted through the storage API before transferring it, rather than assigning untouchedTask.Error only in memory. Reuse the existing FailTask flow demonstrated in the pagination subtest, then keep the TransferTasks2History assertion to verify history transfer of the persisted error.
🤖 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/storage/task_table.go`:
- Around line 443-447: Update the cleanup query in the task-table method using
ExecuteSQLWithNewSession to order terminal tasks by state_update_time ascending
before applying the limit, adding id ascending as a deterministic tie-breaker if
supported. Preserve the existing terminal-state filters and cleanup batch-size
limit.
---
Nitpick comments:
In `@pkg/dxf/framework/storage/table_test.go`:
- Around line 1164-1165: Update the test around TransferTasks2History so the
task error is persisted through the storage API before transferring it, rather
than assigning untouchedTask.Error only in memory. Reuse the existing FailTask
flow demonstrated in the pagination subtest, then keep the TransferTasks2History
assertion to verify history transfer of the persisted error.
🪄 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: 2ac40fb2-3e08-4d22-8ce4-1aa076eb7cfa
📒 Files selected for processing (11)
.agents/skills/tidb-test-guidelines/references/dxf-case-map.mdpkg/dxf/framework/mock/scheduler_mock.gopkg/dxf/framework/mock/task_executor_mock.gopkg/dxf/framework/scheduler/interface.gopkg/dxf/framework/scheduler/scheduler_manager.gopkg/dxf/framework/scheduler/scheduler_manager_nokit_test.gopkg/dxf/framework/scheduler/scheduler_manager_test.gopkg/dxf/framework/scheduler/scheduler_test.gopkg/dxf/framework/storage/table_test.gopkg/dxf/framework/storage/task_table.gopkg/dxf/framework/taskexecutor/interface.go
💤 Files with no reviewable changes (2)
- pkg/dxf/framework/taskexecutor/interface.go
- pkg/dxf/framework/mock/task_executor_mock.go
Contributor
Author
|
/retest |
wjhuang2016
approved these changes
Jul 14, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
joechenrh
reviewed
Jul 14, 2026
| } | ||
|
|
||
| func TestTaskHistoryTable(t *testing.T) { | ||
| t.Run("cleanup tasks are batch limited", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Extract to a separate test
| _, err = sqlexec.ExecSQL(ctx, exec, ` | ||
| insert into mysql.tidb_background_subtask_history | ||
| select * from mysql.tidb_background_subtask | ||
| where task_key in(`+taskIDList+`)`) |
Contributor
There was a problem hiding this comment.
Passing integer here will introduce CAST, thus make this query a full table scan instead of index look up.
root@127.0.0.1 [test]> EXPLAIN SELECT * FROM subtask WHERE task_key = '1';
+-------------------------------+---------+-----------+---------------------------------------------+-------------------------------------------------+
| id | estRows | task | access object | operator info |
+-------------------------------+---------+-----------+---------------------------------------------+-------------------------------------------------+
| IndexLookUp_8 | 10.00 | root | | |
| ├─IndexRangeScan_6(Build) | 10.00 | cop[tikv] | table:subtask, index:idx_task_key(task_key) | range:["1","1"], keep order:false, stats:pseudo |
| └─TableRowIDScan_7(Probe) | 10.00 | cop[tikv] | table:subtask | keep order:false, stats:pseudo |
+-------------------------------+---------+-----------+---------------------------------------------+-------------------------------------------------+
3 rows in set (0.004 sec)
root@127.0.0.1 [test]> EXPLAIN SELECT * FROM subtask WHERE task_key = 1;
+-------------------------+----------+-----------+---------------+---------------------------------------------------+
| id | estRows | task | access object | operator info |
+-------------------------+----------+-----------+---------------+---------------------------------------------------+
| TableReader_8 | 8000.00 | root | | data:Selection_7 |
| └─Selection_7 | 8000.00 | cop[tikv] | | eq(cast(test.subtask.task_key, double BINARY), 1) |
| └─TableFullScan_6 | 10000.00 | cop[tikv] | table:subtask | keep order:false, stats:pseudo |
+-------------------------+----------+-----------+---------------+---------------------------------------------------+
3 rows in set (0.001 sec)
Contributor
There was a problem hiding this comment.
My local test shows that only using string task keys can already get a 4.55× speedup. But since there is no network transmission here, the actual improvement in the cloud environment remains uncertain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #69788, #67640
Problem Summary:
DXF cleanup previously selected every finished task in one query and transferred task and subtask history using SQL statements whose count grew with the number of tasks. With many small import jobs, the per-task SQL round trips can make cleanup slower than task completion, while selecting an unbounded cleanup set can produce an excessively large transfer transaction.
What changed and how does it work?
20and a valid range of[1, 1000].failed,reverted, orsucceedstate. Each cleanup invocation processes one batch and leaves any remaining backlog for later invocations.GETandPOSTAPI at/dxf/schedule/task_cleanup_batch_size. The setting is memory-only, resets on restart, and should be read or changed on the current DXF owner.Check List
Tests
each task has 3 subtask, task/subtask meta size is 500B/100B. we can see the transfer method became faster now
Verification:
make bazel_preparego test ./pkg/dxf/framework/proto -run '^TestTaskCleanupBatchSize$' -tags=intest,deadlock -count=1./tools/check/failpoint-go-test.sh pkg/dxf/framework/scheduler -run '^(TestSchedulerCleanupTask|TestCleanUpRoutine)$' -count=1./tools/check/failpoint-go-test.sh pkg/dxf/framework/storage -run '^(TestTaskHistoryTable|TestTransferTasks2HistoryUsesBatchStatements)$' -count=1./tools/check/failpoint-go-test.sh pkg/server/handler/tests -run '^(TestDXFAPI|TestDXFMaintenanceAPINotAvailableInUserKeyspace)$' -tags=intest,deadlock,nextgen -count=1make lintgit diff --check upstream/masterSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Summary