wangyong9999 commented on code in PR #282:
URL: https://github.com/apache/paimon-cpp/pull/282#discussion_r3932446515
##########
src/paimon/core/operation/scan_context.cpp:
##########
@@ -178,13 +179,14 @@ Result<std::unique_ptr<ScanContext>>
ScanContextBuilder::Finish() {
if (impl_->path_.empty()) {
return Status::Invalid("cannot scan with empty table path");
}
+ std::shared_ptr<Executor> executor =
+ impl_->executor_ ? impl_->executor_ : GetGlobalDefaultExecutor();
Review Comment:
Thanks, agreed: a CPU-proportional shared pool is the wrong tool for a
serving path, and `FileStoreScan::ReadFileEntries()` blocking in `CollectAll()`
on that same pool could deadlock once every worker is inside a scan. Pushed
3475256, which drops `GetGlobalDefaultExecutor()` entirely:
- `DefaultExecutor` now starts its workers on the first `Add()` instead of
in the constructor. An executor that never receives a task never spawns a
thread, and shutting down or destroying such an executor only cleans up state;
executors that do run tasks behave exactly as before. `workers_` stays guarded
by the existing `state_->mutex`, no new lock.
- `ScanContextBuilder::Finish()` falls back to a fresh
`CreateDefaultExecutor()` owned by the returned context, the same as
`ReadContextBuilder` does. Nothing is shared across contexts.
Net effect on the per-lookup path: constructing or resetting a builder
creates no thread (previously 8), a context that supplies its own executor
creates none at all, and the four default workers only appear once the scan
actually submits work. Covered by
`DefaultExecutorTest.TestWorkersStartOnFirstTask` / `TestShutdownWithoutTasks`
and `ScanContextTest.TestDefaultExecutorIsCreatedPerContext`; the PR
description is updated to match.
--
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]
