From 88b799e3a5fed9eb198be914768e650189a2493e Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Mon, 11 Mar 2024 18:10:02 -0400 Subject: [PATCH 1/5] Add documentation to thread pool and executor code --- .../s3/RepositoryCredentialsTests.java | 2 +- .../repositories/s3/S3RepositoryTests.java | 2 +- .../repositories/url/URLRepositoryTests.java | 2 +- .../common/util/concurrent/EsExecutors.java | 3 +++ .../blobstore/BlobStoreRepository.java | 8 +++--- .../threadpool/FixedExecutorBuilder.java | 3 +++ .../threadpool/ScalingExecutorBuilder.java | 4 +++ .../elasticsearch/threadpool/ThreadPool.java | 27 +++++++++++++++++++ .../BlobStoreRepositoryRestoreTests.java | 2 +- .../snapshots/SnapshotResiliencyTests.java | 2 +- ...archableSnapshotsPrewarmingIntegTests.java | 4 +-- ...SnapshotRecoveryStateIntegrationTests.java | 2 +- .../SearchableSnapshotDirectoryTests.java | 2 +- 13 files changed, 50 insertions(+), 13 deletions(-) diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java index cf3bc21526bf6..726f01ebb07b1 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java +++ b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java @@ -267,7 +267,7 @@ protected S3Repository createRepository( ) { return new S3Repository(metadata, registry, getService(), clusterService, bigArrays, recoverySettings, s3RepositoriesMetrics) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // eliminate thread name check as we create repo manually on test/main threads } }; diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java index 50470ec499ef6..185a3651dc8b7 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java +++ b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java @@ -132,7 +132,7 @@ private S3Repository createS3Repo(RepositoryMetadata metadata) { S3RepositoriesMetrics.NOOP ) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // eliminate thread name check as we create repo manually on test/main threads } }; diff --git a/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java b/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java index 00abf1e77fd57..0dfe4b2a4aae3 100644 --- a/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java +++ b/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java @@ -43,7 +43,7 @@ private URLRepository createRepository(Settings baseSettings, RepositoryMetadata mock(URLHttpClient.Factory.class) ) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // eliminate thread name check as we create repo manually on test/main threads } }; diff --git a/server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java b/server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java index 5fcb4684d3f8d..14c1d1e9ef6aa 100644 --- a/server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java +++ b/server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java @@ -33,6 +33,9 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +/** + * A collection of static methods to help create different ES Executor types. + */ public class EsExecutors { // although the available processors may technically change, for node sizing we use the number available at launch diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index 52cfa2fd5275f..47b5a990e497a 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -679,7 +679,7 @@ protected BlobStore getBlobStore() { * maintains single lazy instance of {@link BlobContainer} */ protected BlobContainer blobContainer() { - assertSnapshotOrGenericThread(); + assertSnapshotOrGenericOrStatelessThread(); if (lifecycle.started() == false) { throw notStartedException(); @@ -704,7 +704,7 @@ protected BlobContainer blobContainer() { * Public for testing. */ public BlobStore blobStore() { - assertSnapshotOrGenericThread(); + assertSnapshotOrGenericOrStatelessThread(); BlobStore store = blobStore.get(); if (store == null) { @@ -1993,7 +1993,7 @@ public long getRestoreThrottleTimeInNanos() { return restoreRateLimitingTimeInNanos.count(); } - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // The Stateless plugin adds custom thread pools for object store operations assert ThreadPool.assertCurrentThreadPool( ThreadPool.Names.SNAPSHOT, @@ -3537,7 +3537,7 @@ public IndexShardSnapshotStatus.Copy getShardSnapshotStatus(SnapshotId snapshotI @Override public void verify(String seed, DiscoveryNode localNode) { - assertSnapshotOrGenericThread(); + assertSnapshotOrGenericOrStatelessThread(); if (isReadOnly()) { try { latestIndexBlobId(); diff --git a/server/src/main/java/org/elasticsearch/threadpool/FixedExecutorBuilder.java b/server/src/main/java/org/elasticsearch/threadpool/FixedExecutorBuilder.java index 9668228ac0ec3..544b085a7006d 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/FixedExecutorBuilder.java +++ b/server/src/main/java/org/elasticsearch/threadpool/FixedExecutorBuilder.java @@ -24,6 +24,9 @@ /** * A builder for fixed executors. + * + * Builds an Executor with a static number of threads, as opposed to {@link ScalingExecutorBuilder} that dynamically scales the number of + * threads in the pool up and down based on request load. */ public final class FixedExecutorBuilder extends ExecutorBuilder { diff --git a/server/src/main/java/org/elasticsearch/threadpool/ScalingExecutorBuilder.java b/server/src/main/java/org/elasticsearch/threadpool/ScalingExecutorBuilder.java index 07504bc5f9d2e..29a7d5df08b7b 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/ScalingExecutorBuilder.java +++ b/server/src/main/java/org/elasticsearch/threadpool/ScalingExecutorBuilder.java @@ -24,6 +24,10 @@ /** * A builder for scaling executors. + * + * The {@link #build} method will instantiate a java {@link ExecutorService} thread pool that starts with the specified minimum number of + * threads and then scales up to the specified max number of threads as needed for excess work, scaling back when the burst of activity + * stops. As opposed to the {@link FixedExecutorBuilder} that keeps a fixed number of threads alive. */ public final class ScalingExecutorBuilder extends ExecutorBuilder { diff --git a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index 97c9ce755c130..6fed43fc9b558 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -59,13 +59,22 @@ import static java.util.Map.entry; import static org.elasticsearch.core.Strings.format; +/** + * Manages all the Java thread pools we create. {@link Names} contains a list of the thread pools, but plugins can dynamically add more + * thread pools to instantiate. + */ public class ThreadPool implements ReportingService, Scheduler { private static final Logger logger = LogManager.getLogger(ThreadPool.class); public static class Names { + // Often used for quick operations not worth the costs of switching to another thread. Also used to avoid the cost of re-queuing + // work while leveraging the interface for ease of coding. + // This should not be used on Netty transport threads if the task may not always complete quickly. public static final String SAME = "same"; + // All the tasks that do not fit into the other categories. Try to avoid this if possible. public static final String GENERIC = "generic"; + // Important management tasks that keep the cluster from falling apart. We do not want these operations stalled. public static final String CLUSTER_COORDINATION = "cluster_coordination"; public static final String GET = "get"; public static final String ANALYZE = "analyze"; @@ -75,6 +84,8 @@ public static class Names { public static final String SEARCH_COORDINATION = "search_coordination"; public static final String AUTO_COMPLETE = "auto_complete"; public static final String SEARCH_THROTTLED = "search_throttled"; + // Important cluster management tasks. Tasks that manage data, and tasks that report on the cluster health via statistics etc. + // It is important that these tasks do not stall because that would block cluster diagnostics. public static final String MANAGEMENT = "management"; public static final String FLUSH = "flush"; public static final String REFRESH = "refresh"; @@ -194,6 +205,13 @@ public Collection builders() { Setting.Property.NodeScope ); + /** + * Defines and builds the many thread pools delineated in {@link Names}. + * + * @param settings + * @param meterRegistry + * @param customBuilders a list of additional thread pool builders that were defined elsewhere (like a Plugin). + */ @SuppressWarnings({ "rawtypes", "unchecked" }) public ThreadPool(final Settings settings, MeterRegistry meterRegistry, final ExecutorBuilder... customBuilders) { assert Node.NODE_NAME_SETTING.exists(settings); @@ -310,6 +328,7 @@ public ThreadPool(final Settings settings, MeterRegistry meterRegistry, final Ex threadContext = new ThreadContext(settings); + // Now that all the thread pools have been defined, actually build them. final Map executors = new HashMap<>(); for (final Map.Entry entry : builders.entrySet()) { final ExecutorBuilder.ExecutorSettings executorSettings = entry.getValue().getSettings(settings); @@ -892,6 +911,11 @@ void check(long newAbsoluteMillis, long newRelativeNanos) { } } + /** + * Holds a thread pool and additional ES information ({@link Info}) about that Java thread pool ({@link ExecutorService}) instance. + * + * See {@link Names} for a list of thread pools, though there can be more dynamically added via plugins. + */ static class ExecutorHolder { private final ExecutorService executor; public final Info info; @@ -907,6 +931,9 @@ ExecutorService executor() { } } + /** + * The settings used to create a Java ExecutorService thread pool. + */ public static class Info implements Writeable, ToXContentFragment { private final String name; diff --git a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java index 327dc3d4f5fd0..2f05f210aa1ba 100644 --- a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java @@ -227,7 +227,7 @@ private Repository createRepository() { new RecoverySettings(Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) ) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // eliminate thread name check as we create repo manually } }; diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java index edde9f0164a6e..a3912389ffc57 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java @@ -2128,7 +2128,7 @@ public RecyclerBytesStreamOutput newNetworkBytesStream() { recoverySettings ) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // eliminate thread name check as we create repo in the test thread } } diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/full/SearchableSnapshotsPrewarmingIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/full/SearchableSnapshotsPrewarmingIntegTests.java index 0cf6cb93c865b..3465f4f82bdbf 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/full/SearchableSnapshotsPrewarmingIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/full/SearchableSnapshotsPrewarmingIntegTests.java @@ -449,9 +449,9 @@ public Map getRepositories( (metadata) -> new FsRepository(metadata, env, namedXContentRegistry, clusterService, bigArrays, recoverySettings) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { if (enabled.get()) { - super.assertSnapshotOrGenericThread(); + super.assertSnapshotOrGenericOrStatelessThread(); } } diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java index 6800dea01863a..985095a74717d 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java @@ -248,7 +248,7 @@ public Map getRepositories( "test-fs", (metadata) -> new FsRepository(metadata, env, namedXContentRegistry, clusterService, bigArrays, recoverySettings) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // ignore } } diff --git a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java index 9c36d7b762871..dfb71d4d422bc 100644 --- a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java +++ b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java @@ -609,7 +609,7 @@ private void testDirectories( ) { @Override - protected void assertSnapshotOrGenericThread() { + protected void assertSnapshotOrGenericOrStatelessThread() { // eliminate thread name check as we create repo manually on test/main threads } }; From 13494cd9a0ef5c25c8432e226db27f4744a19a17 Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Tue, 26 Mar 2024 14:53:00 -0400 Subject: [PATCH 2/5] updates per review --- .../s3/RepositoryCredentialsTests.java | 2 +- .../repositories/s3/S3RepositoryTests.java | 2 +- .../repositories/url/URLRepositoryTests.java | 2 +- .../blobstore/BlobStoreRepository.java | 8 ++++---- .../elasticsearch/threadpool/ThreadPool.java | 19 ++++++++++++------- .../BlobStoreRepositoryRestoreTests.java | 2 +- .../snapshots/SnapshotResiliencyTests.java | 2 +- ...SnapshotRecoveryStateIntegrationTests.java | 2 +- .../SearchableSnapshotDirectoryTests.java | 2 +- 9 files changed, 23 insertions(+), 18 deletions(-) diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java index 726f01ebb07b1..13e582598a2d2 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java +++ b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java @@ -267,7 +267,7 @@ protected S3Repository createRepository( ) { return new S3Repository(metadata, registry, getService(), clusterService, bigArrays, recoverySettings, s3RepositoriesMetrics) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // eliminate thread name check as we create repo manually on test/main threads } }; diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java index 185a3651dc8b7..ff61504d6c525 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java +++ b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java @@ -132,7 +132,7 @@ private S3Repository createS3Repo(RepositoryMetadata metadata) { S3RepositoriesMetrics.NOOP ) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // eliminate thread name check as we create repo manually on test/main threads } }; diff --git a/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java b/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java index 0dfe4b2a4aae3..a02bff59988d8 100644 --- a/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java +++ b/modules/repository-url/src/test/java/org/elasticsearch/repositories/url/URLRepositoryTests.java @@ -43,7 +43,7 @@ private URLRepository createRepository(Settings baseSettings, RepositoryMetadata mock(URLHttpClient.Factory.class) ) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // eliminate thread name check as we create repo manually on test/main threads } }; diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index d4cab32783b92..5a33a958646df 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -680,7 +680,7 @@ protected BlobStore getBlobStore() { * maintains single lazy instance of {@link BlobContainer} */ protected BlobContainer blobContainer() { - assertSnapshotOrGenericOrStatelessThread(); + assertSnapshotOrStatelessPermittedThreadPool(); if (lifecycle.started() == false) { throw notStartedException(); @@ -705,7 +705,7 @@ protected BlobContainer blobContainer() { * Public for testing. */ public BlobStore blobStore() { - assertSnapshotOrGenericOrStatelessThread(); + assertSnapshotOrStatelessPermittedThreadPool(); BlobStore store = blobStore.get(); if (store == null) { @@ -1994,7 +1994,7 @@ public long getRestoreThrottleTimeInNanos() { return restoreRateLimitingTimeInNanos.count(); } - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // The Stateless plugin adds custom thread pools for object store operations assert ThreadPool.assertCurrentThreadPool( ThreadPool.Names.SNAPSHOT, @@ -3539,7 +3539,7 @@ public IndexShardSnapshotStatus.Copy getShardSnapshotStatus(SnapshotId snapshotI @Override public void verify(String seed, DiscoveryNode localNode) { - assertSnapshotOrGenericOrStatelessThread(); + assertSnapshotOrStatelessPermittedThreadPool(); if (isReadOnly()) { try { latestIndexBlobId(); diff --git a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index 0687371f34374..6c3c9f8a02829 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -68,13 +68,19 @@ public class ThreadPool implements ReportingService, Scheduler { private static final Logger logger = LogManager.getLogger(ThreadPool.class); public static class Names { - // Often used for quick operations not worth the costs of switching to another thread. Also used to avoid the cost of re-queuing - // work while leveraging the interface for ease of coding. - // This should not be used on Netty transport threads if the task may not always complete quickly. + /** + * Often used for quick operations not worth the costs of switching to another thread. Also used to avoid the cost of re-queuing + * work while leveraging the interface for ease of coding. This should not be used on Netty transport threads if the task may not + * always complete quickly. + */ public static final String SAME = "same"; - // All the tasks that do not fit into the other categories. Try to avoid this if possible. + /** All the tasks that do not fit into the other categories. Try to avoid this if possible. */ public static final String GENERIC = "generic"; - // Important management tasks that keep the cluster from falling apart. We do not want these operations stalled. + /** + * Important management tasks that keep the cluster from falling apart. + * This thread pool ensures cluster coordination tasks do not get blocked by less critical tasks and can continue to make progress. + * This thread pool also defaults to a single thread, reducing contention on the Coordinator mutex. + */ public static final String CLUSTER_COORDINATION = "cluster_coordination"; public static final String GET = "get"; public static final String ANALYZE = "analyze"; @@ -84,8 +90,7 @@ public static class Names { public static final String SEARCH_COORDINATION = "search_coordination"; public static final String AUTO_COMPLETE = "auto_complete"; public static final String SEARCH_THROTTLED = "search_throttled"; - // Important cluster management tasks. Tasks that manage data, and tasks that report on the cluster health via statistics etc. - // It is important that these tasks do not stall because that would block cluster diagnostics. + /** Cluster management tasks. Tasks that manage data, and tasks that report on the cluster health via statistics etc. */ public static final String MANAGEMENT = "management"; public static final String FLUSH = "flush"; public static final String REFRESH = "refresh"; diff --git a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java index 2f05f210aa1ba..0e4818701c5f5 100644 --- a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryRestoreTests.java @@ -227,7 +227,7 @@ private Repository createRepository() { new RecoverySettings(Settings.EMPTY, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) ) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // eliminate thread name check as we create repo manually } }; diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java index 0abead61efc26..9fcdeb1bf57e5 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java @@ -2133,7 +2133,7 @@ public RecyclerBytesStreamOutput newNetworkBytesStream() { recoverySettings ) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // eliminate thread name check as we create repo in the test thread } } diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java index 985095a74717d..4a15d00bc8168 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java @@ -248,7 +248,7 @@ public Map getRepositories( "test-fs", (metadata) -> new FsRepository(metadata, env, namedXContentRegistry, clusterService, bigArrays, recoverySettings) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // ignore } } diff --git a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java index dfb71d4d422bc..c54ead2bdbc45 100644 --- a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java +++ b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectoryTests.java @@ -609,7 +609,7 @@ private void testDirectories( ) { @Override - protected void assertSnapshotOrGenericOrStatelessThread() { + protected void assertSnapshotOrStatelessPermittedThreadPool() { // eliminate thread name check as we create repo manually on test/main threads } }; From d661f030628909e2951262bca708270dd0320021 Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Wed, 27 Mar 2024 11:46:17 -0400 Subject: [PATCH 3/5] update per review --- .../org/elasticsearch/threadpool/ThreadPool.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index 6c3c9f8a02829..34efb067faa2d 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -67,14 +67,21 @@ public class ThreadPool implements ReportingService, Scheduler { private static final Logger logger = LogManager.getLogger(ThreadPool.class); + /** + * List of names that identify Java thread pools that are created in {@link ThreadPool#ThreadPool}. + */ public static class Names { /** + * Specifies that the task being scheduled should run on the calling thread, not actually a different thread pool. * Often used for quick operations not worth the costs of switching to another thread. Also used to avoid the cost of re-queuing - * work while leveraging the interface for ease of coding. This should not be used on Netty transport threads if the task may not - * always complete quickly. + * work while leveraging the same interface for ease of coding. This should not be used on Netty transport threads if the task may + * not always complete quickly. */ public static final String SAME = "same"; - /** All the tasks that do not fit into the other categories. Try to avoid this if possible. */ + /** + * All the tasks that do not fit into the other categories should use this thread pool. Try to pick one of the other more specific + * thread pools if possible. + */ public static final String GENERIC = "generic"; /** * Important management tasks that keep the cluster from falling apart. From ab706480f50da3d8ab26c9dfef582b6dd9f4193b Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Wed, 27 Mar 2024 11:53:19 -0400 Subject: [PATCH 4/5] touch up --- .../main/java/org/elasticsearch/threadpool/ThreadPool.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index 34efb067faa2d..24e19550ced0e 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -79,8 +79,8 @@ public static class Names { */ public static final String SAME = "same"; /** - * All the tasks that do not fit into the other categories should use this thread pool. Try to pick one of the other more specific - * thread pools if possible. + * All the tasks that do not relate to the purpose of one of the other thread pools should use this thread pool. Try to pick one of + * the other more specific thread pools where possible. */ public static final String GENERIC = "generic"; /** From 04391fc092af1ec489b23e87879f3eadec5ae11b Mon Sep 17 00:00:00 2001 From: Dianna Hohensee Date: Mon, 8 Apr 2024 17:30:28 -0400 Subject: [PATCH 5/5] enhance MANAGEMENT comment --- .../main/java/org/elasticsearch/threadpool/ThreadPool.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java index 24e19550ced0e..8de44d689c39c 100644 --- a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java +++ b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java @@ -97,7 +97,10 @@ public static class Names { public static final String SEARCH_COORDINATION = "search_coordination"; public static final String AUTO_COMPLETE = "auto_complete"; public static final String SEARCH_THROTTLED = "search_throttled"; - /** Cluster management tasks. Tasks that manage data, and tasks that report on the cluster health via statistics etc. */ + /** + * Cluster management tasks. Tasks that manage data, and tasks that report on cluster health via statistics etc. + * Not a latency sensitive thread pool: some tasks may time be long-running; and the thread pool size is limited / relatively small. + */ public static final String MANAGEMENT = "management"; public static final String FLUSH = "flush"; public static final String REFRESH = "refresh";