Copilot commented on code in PR #62044:
URL: https://github.com/apache/doris/pull/62044#discussion_r3026411267
##########
be/src/exec/scan/task_executor/time_sharing/time_sharing_task_executor.cpp:
##########
@@ -553,7 +553,30 @@ void TimeSharingTaskExecutor::_dispatch_thread() {
_running_splits.insert(split);
}
- Result<SharedListenableFuture<Void>> blocked_future_result =
split->process();
+ auto blocked_future_result = [&]() ->
Result<SharedListenableFuture<Void>> {
+ try {
+ doris::enable_thread_catch_bad_alloc++;
+ Defer defer {[&]() { doris::enable_thread_catch_bad_alloc--;
}};
+ return split->process();
+ } catch (const doris::Exception& e) {
+ if (e.code() == doris::ErrorCode::MEM_ALLOC_FAILED) {
+ return unexpected(Status::MemoryLimitExceeded(
+ "PreCatch error code:{}, {}, __FILE__:{},
__LINE__:{}, "
+ "__FUNCTION__:{}",
+ e.code(), e.to_string(), __FILE__, __LINE__,
__PRETTY_FUNCTION__));
Review Comment:
In the MEM_ALLOC_FAILED branch, constructing a fresh
`Status::MemoryLimitExceeded(...)` drops the original exception stack (from
`e.to_status()`) and replaces it with the catch-site file/line, which can make
OOM debugging harder. Consider starting from `Status st = e.to_status();` then
remapping the code to `MEM_LIMIT_EXCEEDED` (e.g., via `st.set_code(...)`) and
optionally prepending context, so the original stack is preserved while still
reporting MemoryLimitExceeded.
```suggestion
Status st = e.to_status();
st.set_code(doris::ErrorCode::MEM_LIMIT_EXCEEDED);
return unexpected(st);
```
##########
be/src/exec/scan/task_executor/time_sharing/time_sharing_task_executor.cpp:
##########
@@ -553,7 +553,30 @@ void TimeSharingTaskExecutor::_dispatch_thread() {
_running_splits.insert(split);
}
- Result<SharedListenableFuture<Void>> blocked_future_result =
split->process();
+ auto blocked_future_result = [&]() ->
Result<SharedListenableFuture<Void>> {
+ try {
+ doris::enable_thread_catch_bad_alloc++;
+ Defer defer {[&]() { doris::enable_thread_catch_bad_alloc--;
}};
+ return split->process();
+ } catch (const doris::Exception& e) {
+ if (e.code() == doris::ErrorCode::MEM_ALLOC_FAILED) {
+ return unexpected(Status::MemoryLimitExceeded(
+ "PreCatch error code:{}, {}, __FILE__:{},
__LINE__:{}, "
+ "__FUNCTION__:{}",
+ e.code(), e.to_string(), __FILE__, __LINE__,
__PRETTY_FUNCTION__));
+ }
+ return unexpected(e.to_status());
+ } catch (const std::exception& e) {
Review Comment:
This try/catch block duplicates the exception-to-Status mapping logic
already implemented in `common/exception.h` (including the
`enable_thread_catch_bad_alloc` guard and the "PreCatch" message). To avoid
future divergence, consider factoring the conversion into a shared helper
(e.g., a function that converts an `Exception`/`std::exception` to `Status`)
and reuse it here for the `Result<...>` error path.
--
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]
