Register thread pool settings #18674

jasontedor · 2016-06-01T02:13:43Z

This commit refactors the handling of thread pool settings so that the
individual settings can be registered rather than registering the top
level group. With this refactoring, individual plugins must now register
their own settings for custom thread pools that they need, but a
dedicated API is provided for this in the thread pool module. This
commit also renames the prefix on the thread pool settings from
"threadpool" to "thread_pool". This enables a hard break on the settings
so that:

some of the settings can be given more sensible names (e.g., the max
number of threads in a scaling thread pool is now named "max" instead
of "size")
change the soft limit on the number of threads in the bulk and
indexing thread pools to a hard limit
the settings names for custom plugins for thread pools can be
prefixed (e.g., "xpack.watcher.thread_pool.size")
thread pool settings are now node-level settings

Relates #18613, closes #9216

This commit refactors the handling of thread pool settings so that the individual settings can be registered rather than registering the top level group. With this refactoring, individual plugins must now register their own settings for custom thread pools that they need, but a dedicated API is provided for this in the thread pool module. This commit also renames the prefix on the thread pool settings from "threadpool" to "thread_pool". This enables a hard break on the settings so that: - some of the settings can be given more sensible names (e.g., the max number of threads in a scaling thread pool is now named "max" instead of "size") - change the soft limit on the number of threads in the bulk and indexing thread pools to a hard limit - the settings names for custom plugins for thread pools can be prefixed (e.g., "xpack.watcher.thread_pool.size")

s1monw · 2016-06-01T08:33:00Z

@jasontedor I think if we are breaking things here hard we should go and make them node level settings and prevent them from being updated.

jasontedor · 2016-06-01T20:12:57Z

I think if we are breaking things here hard we should go and make them node level settings and prevent them from being updated.

I think that having them as node-level settings makes the most sense, especially when considering heterogeneous clusters. The one downside to the settings not being dynamically updatable is that right now there is an "out" if an executor or backing queue is too small, it can be dynamically increased as solution to the problem.

I'm happy to remove this though; do you think that should be done in this PR or a follow-up @s1monw?

* master: Update reindex.asciidoc (#18687) Add more logging to reindex rethrottle [TEST] Increase timeout to wait on folder deletion in IndexWithShadowReplicasIT [TEST] Fix tests that rely on assumption that data dirs are removed after index deletion (#18681) Acknowledge index deletion requests based on standard cluster state acknowledgment (#18602) Register "cloud.node.auto_attributes" setting (#18678) Fix StoreRecoveryTests after 6.0.1 upgrade. Upgrade to Lucene 6.0.1. ingest: added `ignore_failure` option to all processors Throw IllegalStateException when handshake fails due to version or cluster mismatch (#18676) AggregatorBuilder and PipelineAggregatorBuilder do not need generics. #18368 Move PageCacheRecycler into BigArrays (#18666) Index Template: parse and validate mappings in template when template puts [TEST] Set BWC version to 5.0.0-SNAP since this is it's min compat version Make cluster health classes immutable and have them implement Writeable instead of Streamable Fix javadoc that stated a throws clause that didn't exist. Clarify the semantics of the BlobContainer interface Build: Rework eclipse settings copy so it does not get automatically cleaned

This commit removes the ability for thread pool settings to be adjusted dynamically. With this commit, thread pool settings are now node-level settings and can not be modified via the cluster settings API.

This commit removes some unused imports that are no longer needed after refactoring thread pool settings.

This commit modifies settings prefixes so that the name is not duplicated (e.g., "xpack.watcher.thread_pool.watcher" under the previous code).

This commit removes the now unnecessary retired executors field from ThreadPool after retiring executors are no longer a thing due to remove dynamic thread pool settings.

s1monw · 2016-06-02T15:52:31Z

core/src/main/java/org/elasticsearch/threadpool/FixedExecutorBuilder.java

+                Setting.Property.Dynamic, Setting.Property.NodeScope);
+        final String queueSizeKey = settingsKey(prefix, name, "queue_size");
+        this.queueSizeSetting =
+            Setting.intSetting(queueSizeKey, queueSize, Setting.Property.Dynamic, Setting.Property.NodeScope);


Setting.Property.Dynamic, can go away

That was a mistake, I removed them from the scaling executor settings but forget them here. I'm glad that you noticed. Removed in 76026f1.

This commit removes the volatile keyword from the executors field; this is no longer needed as the executors can no longer be updated.

s1monw · 2016-06-02T15:54:33Z

core/src/main/java/org/elasticsearch/threadpool/ScalingExecutorBuilder.java

+    }
+
+}
+


newlines FTW?

You get a newline, and you get a newline, and you get a newline, everyone gets a newline! 😄

Removed in 2f29247.

This commit removes some unnecessary trailing newlines from o/e/t/ScalingExecutorBuilder.java.

s1monw · 2016-06-02T15:55:58Z

core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java


 /**
 *
 */
-public class ThreadPool extends AbstractComponent implements Closeable {
+public class ThreadPool extends AbstractLifecycleComponent<ThreadPool> {


do we really need to extend this class? I wanna get rid of it? We should set things up in a ctor and impl. Closeable IMO

It's tricky. I want to construct the executors map once, making it effectively final (although it can not be actually final right now since we do not build it in the constructor). Given this desire, we can not build it in the constructor because we need to wait until all plugins have had a chance to register their custom thread pool settings. But the thread pool instance is needed earlier than this happens because it's needed for the MonitorService which is in turn needed for the NodeModule which must be built before we start processing plugins. The timeline is thus:

constructor runs < inject into MonitorService < inject into NodeModule < plugins process modules < start thread pool to create executors

I looked at options around this but I didn't like any of them, they were all super hacky and involved registering suppliers that used the injector.

can we just get rid of custom threadpools instead?

I think this patch could work just fine?

diff --git a/core/src/main/java/org/elasticsearch/node/Node.java b/core/src/main/java/org/elasticsearch/node/Node.java index 8a1df50..94a0ab1 100644 --- a/core/src/main/java/org/elasticsearch/node/Node.java +++ b/core/src/main/java/org/elasticsearch/node/Node.java @@ -98,6 +98,7 @@ import org.elasticsearch.search.SearchService; import org.elasticsearch.snapshots.SnapshotShardsService; import org.elasticsearch.snapshots.SnapshotsService; import org.elasticsearch.tasks.TaskResultsService; +import org.elasticsearch.threadpool.ExecutorBuilder; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.threadpool.ThreadPoolModule; import org.elasticsearch.transport.TransportService; @@ -210,12 +211,12 @@ public class Node implements Closeable { throw new IllegalStateException("Failed to created node environment", ex); } final NetworkService networkService = new NetworkService(settings); - final ThreadPool threadPool = new ThreadPool(settings); + final List<ExecutorBuilder<?>> executorBuilders = pluginsService.getExecutorBuilders(); + final ThreadPool threadPool = new ThreadPool(settings, executorBuilders.toArray(new ExecutorBuilder[0])); NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(); boolean success = false; try { - final MonitorService monitorService = new MonitorService(settings, nodeEnvironment, threadPool); ModulesBuilder modules = new ModulesBuilder(); modules.add(new Version.Module(version)); modules.add(new CircuitBreakerModule(settings)); @@ -223,6 +224,7 @@ public class Node implements Closeable { for (Module pluginModule : pluginsService.nodeModules()) { modules.add(pluginModule); } + final MonitorService monitorService = new MonitorService(settings, nodeEnvironment, threadPool); modules.add(new PluginsModule(pluginsService)); SettingsModule settingsModule = new SettingsModule(this.settings); modules.add(settingsModule); diff --git a/core/src/main/java/org/elasticsearch/plugins/Plugin.java b/core/src/main/java/org/elasticsearch/plugins/Plugin.java index 1efc151..695a255 100644 --- a/core/src/main/java/org/elasticsearch/plugins/Plugin.java +++ b/core/src/main/java/org/elasticsearch/plugins/Plugin.java @@ -23,9 +23,12 @@ import org.elasticsearch.common.component.LifecycleComponent; import org.elasticsearch.common.inject.Module; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexModule; +import org.elasticsearch.threadpool.ExecutorBuilder; +import org.elasticsearch.threadpool.ThreadPool; import java.util.Collection; import java.util.Collections; +import java.util.List; /** * An extension point allowing to plug in custom functionality. @@ -80,4 +83,8 @@ public abstract class Plugin { */ @Deprecated public final void onModule(IndexModule indexModule) {} + + public List<ExecutorBuilder<?>> getExecutorBuilders() { + return Collections.emptyList(); + } } diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index f373da6..bb22854 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -40,6 +40,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexModule; +import org.elasticsearch.threadpool.ExecutorBuilder; import java.io.IOException; import java.lang.reflect.InvocationTargetException; @@ -261,6 +262,14 @@ public class PluginsService extends AbstractComponent { return modules; } + public List<ExecutorBuilder<?>> getExecutorBuilders() { + ArrayList<ExecutorBuilder<?>> builders = new ArrayList<>(); + for (Tuple<PluginInfo, Plugin> plugin : plugins) { + builders.addAll(plugin.v2().getExecutorBuilders()); + } + return getExecutorBuilders(); + } + public Collection<Class<? extends LifecycleComponent>> nodeServices() { List<Class<? extends LifecycleComponent>> services = new ArrayList<>(); for (Tuple<PluginInfo, Plugin> plugin : plugins) { diff --git a/core/src/main/java/org/elasticsearch/threadpool/ExecutorBuilder.java b/core/src/main/java/org/elasticsearch/threadpool/ExecutorBuilder.java index 31f3f31..61e5141 100644 --- a/core/src/main/java/org/elasticsearch/threadpool/ExecutorBuilder.java +++ b/core/src/main/java/org/elasticsearch/threadpool/ExecutorBuilder.java @@ -30,7 +30,7 @@ import java.util.List; * * @param <U> the underlying type of the executor settings */ -abstract class ExecutorBuilder<U extends ExecutorBuilder.ExecutorSettings> { +public abstract class ExecutorBuilder<U extends ExecutorBuilder.ExecutorSettings> { private final String name; diff --git a/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index 0b564b2..1d641aa 100644 --- a/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/core/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -151,7 +151,7 @@ public class ThreadPool extends AbstractLifecycleComponent<ThreadPool> { return Collections.unmodifiableCollection(builders.values()); } - public ThreadPool(Settings settings) { + public ThreadPool(Settings settings, ExecutorBuilder<?>... customBuilders) { super(settings); final Map<String, ExecutorBuilder> builders = new HashMap<>(); @@ -175,7 +175,13 @@ public class ThreadPool extends AbstractLifecycleComponent<ThreadPool> { 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))); - this.builders = builders; + for (ExecutorBuilder<?> builder : customBuilders) { + if (builders.containsKey(builder.name())) { + throw new IllegalArgumentException("builder with name: " + builder.name() + " already exists"); + } + builders.put(builder.name(), builder); + } + this.builders = Collections.unmodifiableMap(builders); assert Node.NODE_NAME_SETTING.exists(settings); threadContext = new ThreadContext(settings); @@ -190,10 +196,6 @@ public class ThreadPool extends AbstractLifecycleComponent<ThreadPool> { this.estimatedTimeThread.start(); } - void add(ExecutorBuilder builder) { - builders.put(builder.name(), builder); - } - @Override protected void doStart() { final Map<String, ExecutorHolder> executors = new HashMap<>();

@s1monw I pushed 1fdc2db.

This commit removes the dynamic properties from fixed executor settings.

s1monw · 2016-06-02T15:59:19Z

core/src/main/java/org/elasticsearch/threadpool/ScalingExecutorBuilder.java

+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+public class ScalingExecutorBuilder extends ExecutorBuilder<ScalingExecutorBuilder.ScalingExecutorSettings> {


these look awesome. Can we get some java docs?

I pushed 070e260.

s1monw · 2016-06-02T16:08:45Z

core/src/main/java/org/elasticsearch/threadpool/FixedExecutorBuilder.java

+            info.getQueueSize() == null ? "unbounded" : info.getQueueSize());
+    }
+
+    private static SizeValue queueSizeValue(int queueSize) {


only used once so maybe a local var is easier ie. inline

I pushed b2fb135.

This commit cleans up some simple issues for executor builders: - marks the implementations as final - restricts the visibilty of some methods - adds Javadocs - changes a couple of method names for clarity

This commit inlines the queue size utility method in the fixed executor builder as it was only used in one place.

This commit increases the hard size limit on bulk and indexing threads to one plus the bounded number of processors. This allows for an extra schedulable thread in case one of the others from the pool is blocked on I/O or a lock.

This commit refactors custom thread pool registration. Rather than processing the thread pool module in an onModule method on plugins, we simply ask the plugins for their custom thread pool registration. This simplifies custom thread pool registration and ultimately thread pool construction.

This commit reverts an inadvertent import order chance in AbstractClient.java.

* master: (22 commits) Fix recovery throttling to properly handle relocating non-primary shards (#18701) Fix merge stats rendering in RestIndicesAction (#18720) [TEST] mute RandomAllocationDeciderTests.testRandomDecisions Reworked docs for index-shrink API (#18705) Improve painless compile-time exceptions Adds UUIDs to snapshots Add test rethrottle test case for delete-by-query Do not start scheduled pings until transport start Adressing review comments Only filter intial recovery (post API) when shrinking an index (#18661) Add tests to check that toQuery() doesn't return null Removing handling of null lucene query where we catch this at parse time Handle empty query bodies at parse time and remove EmptyQueryBuilder Mute failing assertions in IndexWithShadowReplicasIT until fix Remove allow running as root Add upgrade-not-supported warning to alpha release notes remove unrecognized javadoc tag from matrix aggregation module set ValuesSourceConfig fields as private Adding MultiValuesSource support classes and documentation to matrix stats agg module New Matrix Stats Aggregation module ...

s1monw · 2016-06-03T14:23:05Z

core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

@@ -187,7 +187,7 @@ public synchronized Settings applySettings(Settings newSettings) {
        addSettingsUpdater(setting.newUpdater(consumer, logger, validator));
    }

-    synchronized void addSettingsUpdater(SettingUpdater<?> updater) {
+    public synchronized void addSettingsUpdater(SettingUpdater<?> updater) {


can we move this back to pkg private?

Done in a0afb92.

This commit reverts the method AbstractScopedSettings#addSettingsUpdater to being package-private. This method was previously made public for an earlier refactoring but that refactoring has been undone after making thread pool settings non-dynamic.

s1monw · 2016-06-03T14:27:50Z

core/src/main/java/org/elasticsearch/plugins/Plugin.java

@@ -80,4 +83,8 @@ public void onIndexModule(IndexModule indexModule) {}
     */
    @Deprecated
    public final void onModule(IndexModule indexModule) {}
+
+    public List<ExecutorBuilder<?>> getExecutorBuilders(Settings settings) {


can you put some javadocs on this?

s1monw · 2016-06-03T14:31:33Z

I added some minors LGTM otehrwise - no need for another round of review.

This commit renames two methods in ExecutorBuilder to add "get" prefixes to their method names.

This commit adds Javadocs for the extension point for plugins to register custom thread pools.

s1monw · 2016-06-03T21:24:40Z

LGTM thanks @jasontedor

This commit registers the estimated time interval setting, the interval at which the estimated time thread updates the estimated time.

jasontedor added >enhancement >breaking review :Core/Infra/Settings Settings infrastructure and APIs :Core/Infra/Core Core issues without another label v5.0.0-alpha4 labels Jun 1, 2016

jasontedor mentioned this pull request Jun 1, 2016

Improve thread pools #18613

Open

7 tasks

clintongormley removed the >enhancement label Jun 1, 2016

jasontedor added 5 commits June 1, 2016 16:15

Remove dynamic thread pool settings

a219971

This commit removes the ability for thread pool settings to be adjusted dynamically. With this commit, thread pool settings are now node-level settings and can not be modified via the cluster settings API.

Remove unused imports

ae9bea0

This commit removes some unused imports that are no longer needed after refactoring thread pool settings.

Modify thread pool settings prefixes

cc935f0

This commit modifies settings prefixes so that the name is not duplicated (e.g., "xpack.watcher.thread_pool.watcher" under the previous code).

Remove unused retired executors field

08b14d0

This commit removes the now unnecessary retired executors field from ThreadPool after retiring executors are no longer a thing due to remove dynamic thread pool settings.

s1monw reviewed Jun 2, 2016
View reviewed changes

Remove volatile keyword from executors field

ccabd8c

This commit removes the volatile keyword from the executors field; this is no longer needed as the executors can no longer be updated.

s1monw reviewed Jun 2, 2016
View reviewed changes

Remove newlines in o.e.t.ScalingExecutorBuilder

2f29247

This commit removes some unnecessary trailing newlines from o/e/t/ScalingExecutorBuilder.java.

s1monw reviewed Jun 2, 2016
View reviewed changes

Remove dynamic from fixed executor settings

76026f1

This commit removes the dynamic properties from fixed executor settings.

s1monw reviewed Jun 2, 2016
View reviewed changes

jasontedor added 6 commits June 2, 2016 12:31

Cleanup executor builders

070e260

This commit cleans up some simple issues for executor builders: - marks the implementations as final - restricts the visibilty of some methods - adds Javadocs - changes a couple of method names for clarity

Inline queue size method in fixed executor builder

b2fb135

This commit inlines the queue size utility method in the fixed executor builder as it was only used in one place.

Add an extra schedulable bulk and indexing thread

a1e3cb3

This commit increases the hard size limit on bulk and indexing threads to one plus the bounded number of processors. This allows for an extra schedulable thread in case one of the others from the pool is blocked on I/O or a lock.

Revert import order change in AbstractClient

26eb8b1

This commit reverts an inadvertent import order chance in AbstractClient.java.

s1monw reviewed Jun 3, 2016
View reviewed changes

jasontedor added 2 commits June 3, 2016 10:31

Add prefix to method names in ExecutorBuilder

b6bcafa

This commit renames two methods in ExecutorBuilder to add "get" prefixes to their method names.

Add Javadocs for plugin custom thread pools

001c3cf

This commit adds Javadocs for the extension point for plugins to register custom thread pools.

jasontedor mentioned this pull request Jun 6, 2016

ThreadPool.java looks for a config setting it itself prohibits #9216

Closed

Register estimated time interval setting

955d694

This commit registers the estimated time interval setting, the interval at which the estimated time thread updates the estimated time.

jasontedor merged commit da74323 into elastic:master Jun 7, 2016

jasontedor deleted the thread-pool-refactor branch June 7, 2016 02:09

clintongormley added >breaking-java and removed >breaking labels Jul 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register thread pool settings #18674

Register thread pool settings #18674

jasontedor commented Jun 1, 2016 •

edited

Loading

s1monw commented Jun 1, 2016

jasontedor commented Jun 1, 2016 •

edited

Loading

s1monw Jun 2, 2016

jasontedor Jun 2, 2016

s1monw Jun 2, 2016

jasontedor Jun 2, 2016

s1monw Jun 2, 2016

jasontedor Jun 2, 2016

s1monw Jun 2, 2016

s1monw Jun 3, 2016

jasontedor Jun 3, 2016

s1monw Jun 2, 2016

jasontedor Jun 2, 2016 •

edited

Loading

s1monw Jun 2, 2016

jasontedor Jun 2, 2016

s1monw Jun 3, 2016

jasontedor Jun 3, 2016

s1monw Jun 3, 2016

s1monw commented Jun 3, 2016

s1monw commented Jun 3, 2016

Register thread pool settings #18674

Register thread pool settings #18674

Conversation

jasontedor commented Jun 1, 2016 • edited Loading

s1monw commented Jun 1, 2016

jasontedor commented Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor Jun 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jun 3, 2016

s1monw commented Jun 3, 2016

jasontedor commented Jun 1, 2016 •

edited

Loading

jasontedor commented Jun 1, 2016 •

edited

Loading

jasontedor Jun 2, 2016 •

edited

Loading