[fix](be) Account for TaskExecutor admission in scanner scheduling - #66838
[fix](be) Account for TaskExecutor admission in scanner scheduling#66838mrhhsg wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: the fixed-cap case now avoids the private TaskHandle backlog, but one performance/configuration defect remains because the initial admission target becomes the ScannerContext lifetime maximum.
Critical checkpoints:
- Goal and proof: the real-executor test deterministically covers the original four-scanner/two-slot liveness stall, and the fixed
initial=maxpath now reaches EOS. - Scope and reuse: the two-file change is focused. The duplicated effective-capacity formula matches current scheduler construction, though an executor/handle capacity API would be safer when addressing the inline issue.
- Concurrency: executor workers, the pipeline consumer,
_transfer_lock, scheduler lock, and handle/executor locks were traced end to end. No new lock-order, atomic-order, or wake-up defect was found at a fixed cap. - Lifecycle and errors: init failure, non-EOS park/re-enqueue, both EOS replacement orders, cancellation, stop, destruction, and repeated handle removal retain cleanup/progress paths.
- Configuration, parallel paths, and compatibility: existing startup-immutable initial/max settings match for internal and external task-executor schedulers; thread-pool and one-split paths are appropriately unaffected. There are no protocol, storage-format, symbol, or rolling-upgrade changes.
- Tests: test state restoration, scheduler lifetime, dependency polling, and expected row/EOS checks are sound. The missing
initial < maxcase is the gap identified inline. No build or test was run in this review because the review runner forbids builds;git diff --checkpassed. - Observability, persistence, and writes: the profile records the resulting max concurrency; no additional logging/metric need was found. Transactions, persistence, data writes, and FE-BE variable propagation are not involved.
- Performance: the accepted inline issue can pin a file scan at the initial target even after the handle raises its live target. No other CPU, memory, or redundant-work issue survived review.
- User focus: no additional focus was provided.
Review status: converged after two rounds of normal full review plus separate risk-focused scans. One inline issue is submitted; every other candidate was deduplicated or dismissed with code evidence.
| // ScannerContext counts every submitted task as in-flight, while TaskExecutor keeps | ||
| // submissions beyond this limit in a private per-handle queue. Do not create that hidden | ||
| // backlog: a non-EOS scanner may be parked by ScannerContext while it still owns a task | ||
| // handle slot, so queued tasks cannot provide a wake-up if every admitted scanner parks. |
There was a problem hiding this comment.
The assignment immediately below turns task_executor_initial_max_concurrency_per_task into a lifetime cap for the context. That setting is documented as an initial target, and SplitConcurrencyController plus test_min_concurrency_per_task_when_target_concurrency_increases explicitly support growing it. With initial=2, max=16, and a file scan eligible for 16 scanners, the handle target can grow but _pull_next_scan_task() can never submit a third occupied slot after this assignment, so the scan stays pinned at two scanners. The new test uses initial=max=2, which hides this regression. Please preserve the eventual local/configured maximum while coordinating submissions with the handle's current admission capacity (or otherwise fixing the hidden-slot accounting), and add an initial < max test that proves both liveness and scale-up.
16f5e3d to
2e59af3
Compare
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ScannerContext counted every submitted scanner task as active even when TaskExecutor kept excess leaf splits in its per-task admission queue. If admitted non-EOS scanners yielded while retaining their TaskExecutor slots, the hidden queued tasks could keep ScannerContext's scheduling margin closed and prevent those scanners from being re-enqueued. Expose the unadmitted leaf split count from TaskHandle and use the admission-aware margin only to re-enqueue splits already owned by TaskExecutor. First-time scanner submissions continue to use the original total in-flight margin and cannot consume admission-aware capacity.
### Release note
Fix a possible file scan scheduling stall when ScannerContext concurrency exceeds TaskExecutor admission.
### Check List (For Author)
- Test:
- Unit Test: ScannerContextTest and TimeSharingTaskExecutorTest
- Behavior changed: Yes (yielded admitted scanners can resume without increasing first-time TaskExecutor submissions)
- Does this need documentation: No
2e59af3 to
b635ab2
Compare
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
ScannerContextcounted every scanner submitted toTaskExecutoras progress-capable in-flight work. However,TaskExecutoradmits only the TaskHandle target concurrency and keeps excess leaf splits in a per-handle admission queue. When an admitted non-EOS scanner yielded, the queued splits could keep the ScannerContext scheduling margin closed, preventing the yielded scanner from being re-enqueued even though it still owned an admission slot. If every admitted scanner reached that state, neither scheduler could make progress.This change exposes the number of accepted but unadmitted leaf splits through
TaskHandleand separates two scheduling budgets:is_first_schedule=false) and therefore callsre_enqueue_split. It cannot create a new split or enlarge the TaskExecutor admission backlog.The ScannerContext limit is therefore no longer reduced to the TaskExecutor admission limit, while TaskExecutor can continue to adapt its own admission target without blocking the continuation of already-admitted scanners.
A deterministic integration unit test uses six yielding scanners, a ScannerContext limit of four, and a TaskHandle admission limit of two. It verifies that the initial excess splits enter the admission queue, yielded scanners remain schedulable, all six scanners reach EOS, and total submitted work never exceeds the ScannerContext limit. A focused selection assertion also verifies that admission-aware scheduling refuses a first-time scanner and only selects an existing split.
Release note
Fix a possible file scan scheduling stall when ScannerContext concurrency exceeds TaskExecutor admission.
Check List (For Author)
Test
ENABLE_UNITY_BUILD=OFF ./run-be-ut.sh --run --filter='ScannerContextTest.*:TimeSharingTaskExecutorTest.*' -j 32(43 passed, ASAN UT)ENABLE_UNITY_BUILD=OFF ./run-be-ut.sh --run --filter='ScannerContextTest.task_executor_admission_queue_keeps_scanners_live' -j 32build-support/clang-format.shbuild-support/check-format.shgit diff --checkNOLINTENDinbe/src/core/types.h:576; changed TaskHandle files reported no warnings.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)