mrhhsg commented on code in PR #68610:
URL: https://github.com/apache/doris/pull/68610#discussion_r4267158633
##########
be/src/common/config.cpp:
##########
@@ -335,9 +335,9 @@ DEFINE_Int32(task_executor_max_concurrency_per_task, "-1");
DEFINE_Int32(task_executor_initial_max_concurrency_per_task, "-1");
// Enable task executor in internal table scan.
-DEFINE_Bool(enable_task_executor_in_internal_table, "true");
+DEFINE_Bool(enable_task_executor_in_internal_table, "false");
// Enable task executor in external table scan.
-DEFINE_Bool(enable_task_executor_in_external_table, "true");
+DEFINE_Bool(enable_task_executor_in_external_table, "false");
Review Comment:
Re-checked at the rebased head `3b7492ca5ac` (master `0cfc81c6163`): the
decision to leave this out of this PR stands.
The point about `ThreadPool::init()` is accurate: it calls
`try_create_thread(_min_threads, l)` and discards the status on purpose (the
comment there says a failed thread creation must not fail pool initialization
because threads can be created later on submit), so a positive `min_threads`
does not rule out a pool with zero running workers. That is master's
`ThreadPool` as it is today, for every pool in the BE, and this PR does not
touch `threadpool.cpp`. The generic fix (withdrawing the failed submit's own
task, waiting for pending starts before queueing behind them, keeping the only
worker while tasks are queued) lives on a separate branch with its own tests
and goes into its own PR, where the `ThreadPool` change can be reviewed on its
own.
##########
be/src/util/threadpool.cpp:
##########
@@ -521,8 +528,14 @@ Status ThreadPool::do_submit(std::shared_ptr<Runnable> r,
ThreadPoolToken* token
l.lock();
_num_threads_pending_start--;
if (_num_threads + _num_threads_pending_start == 0) {
- // If we have no threads, we can't do any work.
- return status;
+ // shutdown() may be waiting for the last pending thread to go
away.
+ _no_threads_cond.notify_all();
+ // If we have no threads, we can't do any work. Callers treat
a failed submit as
+ // a task that will never run (for example, an RPC handler
completes its closure
+ // itself), so withdraw the task queued above before returning
the error.
+ LOG(WARNING) << "Thread pool " << _name
Review Comment:
Re-checked at the rebased head `3b7492ca5ac` (master `0cfc81c6163`): the
decision to leave this out of this PR stands.
The interleaving described here (A and B both queue and reserve a start
while the pool has no worker, A's start fails but returns OK because B's start
is still pending, then B's start fails and returns an error with both tasks
still queued) is `ThreadPool::do_submit()` on master, which this PR does not
modify; the scan schedulers only start using the pool that was the default
before the task executor existed. The fix that makes a submit wait for the
outcome of pending starts while the pool has no worker, and withdraw its task
when the last start fails, is kept on a separate branch for its own PR together
with the thread about the retained failed submission.
--
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]
