github-actions[bot] commented on code in PR #61064:
URL: https://github.com/apache/doris/pull/61064#discussion_r2928738745
##########
be/src/exec/scan/scanner_scheduler.cpp:
##########
@@ -152,28 +152,36 @@ void
ScannerScheduler::_scanner_scan(std::shared_ptr<ScannerContext> ctx,
Thread::set_thread_nice_value();
}
#endif
+
+ // we set and get counter according below order, to make sure the counter
is updated before get_block, and the time of get_block is recorded in the
counter.
+ // 1. update_wait_worker_timer to make sure the time of waiting for worker
thread is recorded in the timer
+ // 2. start_scan_cpu_timer to make sure the cpu timer include the time of
open and get_block, which is the real cpu time of scanner
+ // 3. update_scan_cpu_timer when defer, to make sure the cpu timer include
the time of open and get_block, which is the real cpu time of scanner
+ // 4. start_wait_worker_timer when defer, to make sure the time of waiting
for worker thread is recorded in the timer
+
MonotonicStopWatch max_run_time_watch;
max_run_time_watch.start();
scanner->update_wait_worker_timer();
scanner->start_scan_cpu_timer();
- // Counter update need prepare successfully, or it maybe core. For
example, olap scanner
- // will open tablet reader during prepare, if not prepare successfully,
tablet reader == nullptr.
- bool need_update_profile = scanner->has_prepared();
+ bool need_update_profile = true;
auto update_scanner_profile = [&]() {
if (need_update_profile) {
scanner->update_scan_cpu_timer();
scanner->update_realtime_counters();
need_update_profile = false;
}
};
- Defer defer_scanner([&] {
- // WorkloadGroup Policy will check cputime realtime, so that should
update the counter
- // as soon as possible, could not update it on close.
- update_scanner_profile();
- });
+
Status status = Status::OK();
bool eos = false;
+ Defer defer_scanner([&] {
+ if (status.ok() && !eos) {
+ // if status is not ok, it means the scanner is failed, and the
counter may be not updated correctly, so no need to update counter again. if
eos is true, it means the scanner is finished successfully, and the counter is
updated correctly, so no need to update counter again.
+ scanner->start_wait_worker_timer();
Review Comment:
**Minor: `start_wait_worker_timer()` here is redundant**
`start_wait_worker_timer()` resets and restarts the internal `_watch`.
However, `ScannerScheduler::submit()` (line 70 of this file) also calls
`start_wait_worker_timer()` unconditionally when the scan task is re-submitted
to the thread pool. Since `submit()` runs after the current `_scanner_scan`
returns and `push_back_scan_task` is processed, the timer started here is
immediately overwritten.
The actual wait time is measured from the `submit()` call (line 70) to the
next `update_wait_worker_timer()` call (line 164) when a worker picks it up.
##########
be/src/exec/scan/scanner_scheduler.cpp:
##########
@@ -152,28 +152,36 @@ void
ScannerScheduler::_scanner_scan(std::shared_ptr<ScannerContext> ctx,
Thread::set_thread_nice_value();
}
#endif
+
+ // we set and get counter according below order, to make sure the counter
is updated before get_block, and the time of get_block is recorded in the
counter.
+ // 1. update_wait_worker_timer to make sure the time of waiting for worker
thread is recorded in the timer
+ // 2. start_scan_cpu_timer to make sure the cpu timer include the time of
open and get_block, which is the real cpu time of scanner
+ // 3. update_scan_cpu_timer when defer, to make sure the cpu timer include
the time of open and get_block, which is the real cpu time of scanner
+ // 4. start_wait_worker_timer when defer, to make sure the time of waiting
for worker thread is recorded in the timer
+
MonotonicStopWatch max_run_time_watch;
max_run_time_watch.start();
scanner->update_wait_worker_timer();
scanner->start_scan_cpu_timer();
- // Counter update need prepare successfully, or it maybe core. For
example, olap scanner
- // will open tablet reader during prepare, if not prepare successfully,
tablet reader == nullptr.
- bool need_update_profile = scanner->has_prepared();
+ bool need_update_profile = true;
auto update_scanner_profile = [&]() {
if (need_update_profile) {
scanner->update_scan_cpu_timer();
scanner->update_realtime_counters();
need_update_profile = false;
}
};
- Defer defer_scanner([&] {
- // WorkloadGroup Policy will check cputime realtime, so that should
update the counter
- // as soon as possible, could not update it on close.
- update_scanner_profile();
- });
+
Status status = Status::OK();
bool eos = false;
+ Defer defer_scanner([&] {
+ if (status.ok() && !eos) {
Review Comment:
**Bug: `update_scanner_profile()` is no longer called on normal yield path**
The old code called `update_scanner_profile()` unconditionally in the Defer:
```cpp
Defer defer_scanner([&] {
// WorkloadGroup Policy will check cputime realtime, so that should
update the counter
// as soon as possible, could not update it on close.
update_scanner_profile();
});
```
The new code only calls `update_scanner_profile()` when `eos == true` (at
line 342), and the Defer only calls `start_wait_worker_timer()` when
`status.ok() && !eos`. This means:
1. On the **normal yield path** (scanner yields back to scheduler because it
hit the byte threshold, max run time, or other break conditions — where
`status.ok() && !eos`), `update_scan_cpu_timer()` and
`update_realtime_counters()` are **never called**.
2. The old comment explicitly stated: *"WorkloadGroup Policy will check
cputime realtime, so that should update the counter as soon as possible, could
not update it on close."* — This requirement is violated by the new code.
This will cause:
- CPU time for each scanner slice to not be reported to the workload group
until eos, making workload group CPU limits less responsive
- Scan bytes/rows realtime counters to not be updated until eos, potentially
allowing queries to exceed scan byte limits before being detected
**Suggested fix**: Keep calling `update_scanner_profile()` in the Defer (as
before), and additionally call `start_wait_worker_timer()` only on the normal
yield path:
```cpp
Defer defer_scanner([&] {
update_scanner_profile();
if (status.ok() && !eos) {
scanner->start_wait_worker_timer();
}
});
```
--
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]
