morningman commented on code in PR #68712:
URL: https://github.com/apache/doris/pull/68712#discussion_r4176106115
##########
be/src/exec/scan/scanner_context.cpp:
##########
@@ -166,14 +164,17 @@ int ScannerContext::_available_pickup_scanner_count() {
P.adjust_scanners_last_timestamp = now;
auto old_scanners = P.expected_scanners;
- scanners = std::max(min_scanners, scanners);
- scanners = std::min(max_scanners, scanners);
+ // The memory limiter is what adapts: its ceiling shrinks when blocks are
estimated larger or
+ // the query's scan budget is shared by more scan nodes, and grows back
when they are not. Take
+ // the whole ceiling. Clamping the previous value into [minimum, ceiling]
instead never raises
+ // it above the minimum, since expected_scanners starts at zero, so every
scan would keep a
+ // single scanner however much memory there is. The ceiling still wins
over the minimum, and a
+ // scheduler without slack still holds the Context at its minimum in
_get_margin() and
+ // can_admit_scan_task().
Review Comment:
Fixed in ed467c58a4e. `_pull_next_scan_task()` now takes `expected_scanners`
as it is -- `_get_margin()` refreshes it just before, so zero is an allocation,
never a value not yet set -- and refuses a task once something is occupied and
the count has reached the ceiling. That is the rule `can_admit_scan_task()`
follows on the thread-pool path: zero is the ceiling, and one task still runs
when nothing is occupied. Test:
`ScannerContextTest.task_executor_keeps_zero_adaptive_allocation`, through
`schedule_scan_task()`: two scanners running, the allocation falls to zero, the
scanner whose block was consumed goes back to pending while the other is in
flight, and one scanner still starts once nothing is occupied. It fails without
the change.
For the record, the window was wider than two scanners: an instance whose
allocation fell to zero kept whatever it held, up to `_max_scan_concurrency`,
for the rest of the scan, and replaced a finished scanner with a pending one,
while a ceiling of one drained to one. Before this PR an instance held at most
one scanner with default settings, so the mapping showed only with
`min_scanners_concurrency` / `min_file_scanners_concurrency` above one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
