github-actions[bot] commented on code in PR #68712:
URL: https://github.com/apache/doris/pull/68712#discussion_r4175757746
##########
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:
[P1] Keep this behind #68710's BE JVM thread-exit fix. For a one-instance
fluss or paimon scan, assigning the full ceiling can admit all 16 JNI readers
because the limiter budgets native block memory, while BE's JVM has a separate
default 2 GB heap. The PR descriptions document a heavy 16-bucket read
exhausting that heap and a library worker calling `System.exit`, which takes
down BE. #68710 is still open, so merging this head first makes that process
failure reachable by default.
##########
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:
[P2] Preserve a zero per-instance allocation when TaskExecutor replaces a
completed scanner. This assignment can grow instance 1 of a two-instance scan
to two scanners; after larger block samples, `available_scanner_count(1)` can
fall to zero. Consuming one non-EOS result still leaves another task occupied,
but `_get_margin()` requests one and `_pull_next_scan_task()` maps
`expected_scanners == 0` to `_max_scan_concurrency`, so it immediately replaces
the consumed scanner instead of draining to the one progress task. Keep zero as
the ceiling while any task remains occupied, and test that shrink transition.
##########
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:
[P2] Count completed tasks in the TaskExecutor low-memory cap. After this
change admits 16 file scanners, a query can enter low-memory mode with 16
completed blocks queued. Consuming one leaves 15 completed; `_get_margin()`
caps with `4 - _in_flight_tasks_num` (4), omitting those blocks, and its
adaptive margin admits one replacement. The queue can remain at 16 under memory
pressure instead of draining toward four. Use completed-plus-in-flight
accounting as `can_admit_scan_task()` does, and cover this transition.
--
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]
