github-actions[bot] commented on code in PR #64784:
URL: https://github.com/apache/doris/pull/64784#discussion_r3466009877
##########
be/src/util/threadpool.cpp:
##########
@@ -471,6 +471,16 @@ Status ThreadPool::do_submit(std::shared_ptr<Runnable> r,
ThreadPoolToken* token
_num_threads_pending_start++;
}
+ if (need_a_thread && _num_threads + _num_threads_pending_start == 1) {
Review Comment:
This fixes the cold-start failure case for `ThreadPool`, but the copied scan
split executor still has the same queue-before-first-worker-create pattern. In
`TimeSharingTaskExecutor::_do_submit`, the code increments
`_num_threads_pending_start`, calls `_offer_split_unlocked()` (which inserts
the split into `_tokenless->_entries` and increments `_total_queued_tasks`),
and only then calls `_create_thread()` after unlocking. If that first worker
creation fails, `_do_submit()` returns the error with the split still queued;
for first-time scan scheduling `_start_split()` only logs that error, so
`enqueue_splits()` can still look successful and a later worker can run a split
whose submit path failed. Please mirror this first-worker precreation/rollback
logic in `TimeSharingTaskExecutor` too, and add a focused failure-injection
test there so the parallel path does not keep retained executable work after a
submit failure.
##########
be/test/util/threadpool_test.cpp:
##########
@@ -176,6 +177,38 @@ TEST_F(ThreadPoolTest, TestThreadPoolWithNoMinimum) {
EXPECT_EQ(0, _pool->num_threads());
}
+TEST_F(ThreadPoolTest, TestSubmitFailureDoesNotRetainTaskWithoutWorker) {
+ bool enable_debug_points = config::enable_debug_points;
+ config::enable_debug_points = true;
+ DebugPoints::instance()->clear();
+ Defer defer = [enable_debug_points] {
+ DebugPoints::instance()->clear();
+ config::enable_debug_points = enable_debug_points;
+ };
+
+ constexpr auto kPoolName = "submit_failure_without_worker";
Review Comment:
This new call is not clang-format v16 compliant, which is why the Clang
Formatter checks are failing. Running the repo formatter locally rewrites it to
wrap `rebuild_pool_with_builder(` and keep the builder chain on one line:
```cpp
EXPECT_TRUE(rebuild_pool_with_builder(
ThreadPoolBuilder(kPoolName).set_min_threads(0).set_max_threads(1))
.ok());
```
Please run `build-support/clang-format.sh` so the C++ style gate passes.
--
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]
