morningman commented on code in PR #68712:
URL: https://github.com/apache/doris/pull/68712#discussion_r4175919696
##########
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:
Agreed, and that is the order the description asks for (**Merge order**:
merge this after #68710). There is no code dependency between the two, so
nothing changes here; this PR waits for #68710.
One correction to the framing, for the record: merging this first does not
make the exit reachable, it makes it more likely. The exit is in the libraries
-- fluss's `FatalExitExceptionHandler` and `ShutdownableThread`, paimon's
`FatalExitExceptionHandler` -- and it fires whenever a fluss or paimon JNI read
fills BE's JVM heap, at whatever concurrency got it there. On master today that
includes `enable_adaptive_scan=false`, under which
`_available_pickup_scanner_count()` returns `_max_scan_concurrency` and a
one-instance scan runs the same 16 readers (the "adaptive scan off" column of
the description); it was also the default concurrency before #61271. #68710
removes the exit itself: after it, a read that runs the heap out fails its
queries and BE stays up, with the residue #68710 lists under "Not in this PR".
##########
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:
Pre-existing, and untouched by this PR; I'd rather fix it on its own than
here.
The cap has counted only running tasks since it was added in #47462
(`low_memory_mode_scanners() - _num_scheduled_scanners`; #61271 only renamed
the counter), and the comment on it says that is the intent: it limits the
running scanners "So that we will not submit too many scan tasks to scheduler".
Sixteen completed blocks stay queued in low-memory mode on master today with
`enable_adaptive_scan=false` -- `_get_margin()` runs the same arithmetic there,
only without the adaptive cap -- and they did by default before #61271; that is
the trade-off the description names ("the one before #61271"). Counting
completed plus in-flight tasks, as `can_admit_scan_task()` does, would change
low-memory behaviour for scans with adaptive scan off as well, which this PR
does not touch.
It also belongs with two other places where the scan side of low-memory mode
no longer does what `scanner_context.h` describes ("at most 4 scanners running
and every scanner will cache at most 1MB data"): `_scanner_scan()` sets
`raw_bytes_threshold` to 1 MB in low-memory mode but no longer reads it, since
a scan task reads one block, and its `try_reserve()` call sits on the branch
after the first read, which a one-block task never takes. Counting queued
blocks alone would hold an instance at four blocks of `batch_size` rows, not
the 8 MB the comment promises. I'll take the three together in a follow-up and
link it here.
--
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]
