[enhance] Support dynamically modify server thread pool#18047
[enhance] Support dynamically modify server thread pool#18047xiangfu0 merged 5 commits intoapache:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18047 +/- ##
============================================
+ Coverage 63.93% 63.96% +0.03%
Complexity 1594 1594
============================================
Files 3178 3179 +1
Lines 193466 193574 +108
Branches 29880 29891 +11
============================================
+ Hits 123683 123811 +128
+ Misses 60010 59986 -24
- Partials 9773 9777 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
91087a6 to
a9c049c
Compare
There was a problem hiding this comment.
Pull request overview
Adds runtime (cluster-config-driven) resizing for the server query scheduler’s runner/worker thread pools, aiming to let operators tune pinot.query.scheduler.query_runner_threads and pinot.query.scheduler.query_worker_threads without restarting servers.
Changes:
- Register a new
PinotClusterConfigChangeListenerfrom the server starter to react to cluster config updates for query scheduler thread-pool sizing. - Introduce a listener that parses the updated config values and calls
ResourceManager.resizeThreadPools(...). - Update
ResourceManagerto keep references to the underlyingThreadPoolExecutors and support resizing; add/expand unit tests for resizing and listener behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java | Registers the new cluster-config change listener during server startup. |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/resources/ResourceManager.java | Stores the underlying executors and adds dynamic resize support. |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QuerySchedulerThreadPoolConfigChangeListener.java | New listener that watches cluster config keys and triggers thread-pool resizing. |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QueryScheduler.java | Exposes getResourceManager() to allow the server starter to wire the listener. |
| pinot-core/src/test/java/org/apache/pinot/core/query/scheduler/resources/ResourceManagerTest.java | Adds tests for increasing/decreasing/no-op/invalid resizes; ensures cleanup. |
| pinot-core/src/test/java/org/apache/pinot/core/query/scheduler/QuerySchedulerThreadPoolConfigChangeListenerTest.java | New tests verifying listener behavior for relevant/unrelated/invalid config changes. |
| pinot-core/src/test/java/org/apache/pinot/core/query/scheduler/PrioritySchedulerTest.java | Adjusts method visibility/override to match new QueryScheduler#getResourceManager(). |
0f0f982 to
7748e80
Compare
| resizePool(_queryRunnerPool, oldRunnerThreads, newRunnerThreads, "queryRunner"); | ||
| _numQueryRunnerThreads = newRunnerThreads; | ||
|
|
||
| resizePool(_queryWorkerPool, oldWorkerThreads, newWorkerThreads, "queryWorker"); | ||
| _numQueryWorkerThreads = newWorkerThreads; |
There was a problem hiding this comment.
resizeThreadPools() has no synchronization, but it mutates multiple related fields and triggers callbacks that may themselves depend on previous values (e.g., PriorityScheduler uses an old->new delta). If resizeThreadPools() is invoked concurrently, deltas and listener state can become inconsistent. Consider guarding the entire resize operation with synchronization (e.g., synchronize the method or use a lock) so resizing + listener notifications are atomic and ordered.
There was a problem hiding this comment.
This is true, just very low chance to happen.
af652b4 to
270ee86
Compare
| int newRunnerThreads = _resourceManager.getNumQueryRunnerThreads(); | ||
| int newWorkerThreads = _resourceManager.getNumQueryWorkerThreads(); | ||
|
|
||
| if (changedConfigs.contains(QUERY_RUNNER_THREADS_KEY) && clusterConfigs.containsKey(QUERY_RUNNER_THREADS_KEY)) { |
There was a problem hiding this comment.
DefaultClusterConfigChangeHandler reports deleted cluster keys in changedConfigs, but this listener only reparses a value when the key is still present in clusterConfigs.
If an operator removes pinot.query.scheduler.query_runner_threads or pinot.query.scheduler.query_worker_threads to roll back the override (expecting the value to be rollback to the system default)
resizeThreadPools is invoked with the current value instead, so the last live size becomes sticky until another explicit number is pushed.
There was a problem hiding this comment.
Good catch, thanks for pointing this out! I’ve updated the PR accordingly.
| } | ||
| _clusterConfigChangeHandler.registerClusterConfigChangeListener(_segmentOperationsThrottlerSet); | ||
| _clusterConfigChangeHandler.registerClusterConfigChangeListener(keepPipelineBreakerStatsPredicate); | ||
| _clusterConfigChangeHandler.registerClusterConfigChangeListener( |
There was a problem hiding this comment.
There is another usage of query_worker_threads at line 707-711:
// Create a thread pool used for mutable lucene index searches, with size based on query_worker_threads config
LOGGER.info("Initializing lucene searcher thread pool");
int queryWorkerThreads =
_serverConf.getProperty(ResourceManager.QUERY_WORKER_CONFIG_KEY, ResourceManager.DEFAULT_QUERY_WORKER_THREADS);
_realtimeLuceneTextIndexSearcherPool = RealtimeLuceneTextIndexSearcherPool.init(queryWorkerThreads);
There was a problem hiding this comment.
Good point, I didn’t consider this case before. I’ve updated the logic to handle it. Please take another look.
cc @xiangfu0
Signed-off-by: Hongkun Xu <xuhongkun666@163.com>
Signed-off-by: Hongkun Xu <xuhongkun666@163.com>
Signed-off-by: Hongkun Xu <xuhongkun666@163.com>
Signed-off-by: Hongkun Xu <xuhongkun666@163.com>
Signed-off-by: Hongkun Xu <xuhongkun666@163.com>
fcc9c06 to
4fb78f8
Compare
|
A documentation PR has been opened for this change: pinot-contrib/pinot-docs#732 |
…#18047) This PR documents the dynamic server thread pool configuration added in apache/pinot#18047: - `pinot.query.scheduler.query_runner_threads` and `pinot.query.scheduler.query_worker_threads` can now be updated at runtime via Helix cluster config without restarting the server - A PinotClusterConfigChangeListener automatically resizes the thread pools on config change Upstream PR: apache/pinot#18047 Co-authored-by: Pinot Docs Bot <pinot-docs-bot@apache.org>
|
📚 Documentation PR opened: pinot-contrib/pinot-docs#735 |
Make
pinot.query.scheduler.query_runner_threadsandpinot.query.scheduler.query_worker_threadsdynamically adjustable at runtime via Helix cluster config, without requiring a server restart.Previously, tuning these thread pool sizes to match available CPU/IO capacity required editing server.conf and restarting the server. This PR adds a PinotClusterConfigChangeListener that listens for changes to these two keys and resizes the underlying ThreadPoolExecutor pools on the fly.