Add a dedicated threadpool for node connections #30150
Conversation
Add a new dedicated threadpool for establishing node connections, instead of using management thread pool as it's a small and important threadpool used by many other actions.
|
Pinging @elastic/es-core-infra |
|
I wonder if we should have a unit test that verifies we are using a different threadpool. @elastic/es-core-infra thoughts on this and the change itself? |
|
Hi @javanna I added a test to check using the new threadpool. |
|
thanks @PnPie ! @elastic/es-core-infra this is ready for review |
There was a problem hiding this comment.
I didn't realise this was open so I opened
#31546. This looks better, however, so I closed it again. I left a couple of comments. The only other thing is that there are docs to update - see https://github.com/elastic/elasticsearch/pull/31546/files#diff-7bb7428e5b1712bb358a0bb44321d9a6
| @@ -89,13 +89,12 @@ public void connectToNodes(DiscoveryNodes discoveryNodes) { | |||
| if (connected) { | |||
| latch.countDown(); | |||
| } else { | |||
| // spawn to another thread to do in parallel | |||
| threadPool.executor(ThreadPool.Names.MANAGEMENT).execute(new AbstractRunnable() { | |||
| threadPool.executor(ThreadPool.Names.NODE_CONNECTIONS).execute(new AbstractRunnable() { | |||
There was a problem hiding this comment.
I think the ConnectionChecker below could also reasonably run on the NODE_CONNECTIONS threadpool.
| @@ -186,6 +188,7 @@ public ThreadPool(final Settings settings, final ExecutorBuilder<?>... customBui | |||
| builders.put(Names.FETCH_SHARD_STARTED, new ScalingExecutorBuilder(Names.FETCH_SHARD_STARTED, 1, 2 * availableProcessors, TimeValue.timeValueMinutes(5))); | |||
| builders.put(Names.FORCE_MERGE, new FixedExecutorBuilder(settings, Names.FORCE_MERGE, 1, -1)); | |||
| builders.put(Names.FETCH_SHARD_STORE, new ScalingExecutorBuilder(Names.FETCH_SHARD_STORE, 1, 2 * availableProcessors, TimeValue.timeValueMinutes(5))); | |||
| builders.put(Names.NODE_CONNECTIONS, new ScalingExecutorBuilder(Names.NODE_CONNECTIONS, 1, 2 * availableProcessors, TimeValue.timeValueMinutes(5))); | |||
There was a problem hiding this comment.
This feels too small to me, for larger clusters. Connecting to a node is not a resource-intensive operation but it might take a long time to time out, blocking other connection attempts. Perhaps this is a reasonable default and we can note in the docs that larger clusters may prefer to increase this limit?
|
Hi @DaveCTurner I updated the PR. |
|
This looks ok to me but I'm not intimately familiar with this area of the code so I'd rather not give it a final LGTM. Could someone from @elastic/es-core-infra take a look at this please? |
|
Hi @PnPie! Thank you very much for your pull request and the work that you put into it. I am sorry that it fell through the cracks. We recently implemented a change in #35144 that will enable us to open connections asynchronously. This means that there is no need for a dedicated threadpool anymore which in turn makes this PR, unfortunately, obsolete. Therefore, I hope that you understand I close this PR without merging. |
|
Thanks @danielmitterdorfer for replying and explaning the context, so that I can also have a look at the related changes :) |
|
You are welcome and thanks for your understanding. Glad that you find the additional information helpful. :) |
Add a new dedicated threadpool for establishing node connections, instead of using management thread pool as it's a small and important threadpool used by many other operations.
Relates to #29023