fix(core): resolve ScanContextBuilder executor lazily in Finish() - #282
fix(core): resolve ScanContextBuilder executor lazily in Finish()#282wangyong9999 wants to merge 4 commits into
Conversation
ScanContextBuilder::Impl created a DefaultExecutor in its member initializer and again in Reset(), spawning and joining eight worker threads per builder even when the caller supplied an executor. Keep the executor unset until Finish(), which falls back to GetGlobalDefaultExecutor() when nothing was set, so ScanContext still carries a non-null executor.
There was a problem hiding this comment.
Please do not use GlobalDefaultExecutor. It creates a number of threads proportional to the number of CPU cores and may also introduce potential locking issues.
There was a problem hiding this comment.
One concrete failure mode is FileStoreScan::ReadFileEntries(): it submits manifest work to the context executor and then blocks in CollectAll(). If all global workers are already running scans, every worker can wait for child work queued to the same pool and deadlock. Please keep the fallback lazy but private (CreateDefaultExecutor() in Finish()), while leaving Reset() null; that removes the eager thread creation without changing executor isolation.
There was a problem hiding this 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:
DefaultExecutornow starts its workers on the firstAdd()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 existingstate_->mutex, no new lock.ScanContextBuilder::Finish()falls back to a freshCreateDefaultExecutor()owned by the returned context, the same asReadContextBuilderdoes. 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.
Review feedback on apache#282: do not fall back to the process wide GetGlobalDefaultExecutor() in ScanContextBuilder::Finish(). Each context keeps a DefaultExecutor of its own again; the thread churn is removed by having DefaultExecutor spawn its workers on the first Add() instead of in the constructor, so an executor that never receives a task never creates a thread.
paimon-common-test runs every suite in one process, so a worker joined by an earlier test can still be listed in /proc/self/task when the baseline is read. Take the baseline once two consecutive reads agree and compare with <= / >= instead of exact counts.
Pin the isolation explicitly: a context built without WithExecutor() must not fall back to GetGlobalDefaultExecutor().
2 participants
Purpose
Linked issue: none
ScanContextBuilder::Impleagerly callsCreateDefaultExecutor()twice per builderlifetime: once in the member initializer and once more in
Reset(), whichFinish()invokes right after building the context.
DefaultExecutorspawnsDEFAULT_EXECUTOR_THREAD_COUNT(4) worker threads in its constructor and joins them ondestruction, so every
ScanContextBuildercosts eight thread creations even when thecaller supplies its own executor through
WithExecutor().Point-lookup style serving code builds one
ScanContextBuilderper key. Measured withstrace -f -c -e trace=cloneon such a path, each lookup performed about nineclonecalls and eight of them came from this builder.
This change keeps every context on a
DefaultExecutorof its own (no process wideexecutor is introduced) and removes the thread churn in two places:
DefaultExecutorstarts its worker threads on the firstAdd()instead of in theconstructor. 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
receive tasks behave as before.
ScanContextBuilder::Impl::executor_defaults to null andReset()only clears it.Finish()resolves the executor: the one set throughWithExecutor()if any,otherwise a fresh
CreateDefaultExecutor()owned by the returned context. Thismatches what
ReadContextBuilderalready does.With both changes a lookup that supplies its own executor creates no thread at all,
and one that relies on the default scan executor creates the four workers once, when
the scan actually submits work.
Tests
DefaultExecutorTest.TestWorkersStartOnFirstTask(new):CreateDefaultExecutor(4)creates no thread; four appear after the first tasks run; all are gone after the
executor is destroyed (thread count read from
/proc/self/task, Linux only).DefaultExecutorTest.TestShutdownWithoutTasks(new):ShutdownNow()anddestruction of an executor that never ran a task.
ScanContextTest.TestDefaultExecutorIsCreatedPerContext(new): contexts builtwithout
WithExecutor()get distinct non-null executors, none of them the global one, and an executor set on thebuilder only applies to the
Finish()that follows it.DefaultExecutorTest.*cases (10 existing + 2 new) and the 4ScanContextTest.*cases pass locally in a Release build (paimon-common-test,paimon-core-test).API and Format
No public API or storage format change.
DefaultExecutorworker threads now start onthe first submitted task rather than at construction; this is only observable through
thread counts.
Documentation
No.
Generative AI tooling
Generated-by: Claude Code (Claude Fable 5.1)