System index reads in separate threadpool #57936
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit moves all system index reads into their own threadpool.
jaymode
added
>enhancement
WIP
:Core/Infra/Core
|
This is still a WIP as I believe there are some points that will require being addressed and some discussion around others. To do:
|
|
I took this approach and ran it in Rally. There is a performance hit here and some values are significant, so it looks like there is some work to do to understand this more. |
|
After some iterations, the performance impact has been reduced to basically noise. Regarding the shape of the system index threadpool, @jimczi provided some feedback:
This is reasonable and I believe we may be able to do this as long as we do not run many long searches on these threads. I plan to test using |
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
jimczi
approved these changes
Jul 29, 2020
| @@ -296,7 +297,8 @@ public IndexShard( | |||
| final List<IndexingOperationListener> listeners, | |||
| final Runnable globalCheckpointSyncer, | |||
| final RetentionLeaseSyncer retentionLeaseSyncer, | |||
| final CircuitBreakerService circuitBreakerService) throws IOException { | |||
| final CircuitBreakerService circuitBreakerService, | |||
| final boolean isSystem) throws IOException { | |||
There was a problem hiding this comment.
Isn't it possible to check the metadata to retrieve this information through the already provided IndexSettings ?
There was a problem hiding this comment.
Yes it is; I missed that metadata was available. Addressed in cffb69d
| @@ -180,6 +182,7 @@ public ThreadPool(final Settings settings, final ExecutorBuilder<?>... customBui | |||
| builders.put(Names.FORCE_MERGE, new FixedExecutorBuilder(settings, Names.FORCE_MERGE, 1, -1, false)); | |||
| builders.put(Names.FETCH_SHARD_STORE, | |||
| new ScalingExecutorBuilder(Names.FETCH_SHARD_STORE, 1, 2 * allocatedProcessors, TimeValue.timeValueMinutes(5))); | |||
| builders.put(Names.SYSTEM_READ, new FixedExecutorBuilder(settings, Names.SYSTEM_READ, halfProcMaxAt5, 2000, false)); | |||
There was a problem hiding this comment.
Since search should be fast on these indices I wonder if a queue size of 1000 is not enough ?
There was a problem hiding this comment.
I combined the queue size for get and search here since we share the threadpool for both types of operations
There was a problem hiding this comment.
ok thanks for explaining. The query and get request are lightweight so 2000 should be ok.
| @@ -82,6 +82,7 @@ | |||
| public static final String FORCE_MERGE = "force_merge"; | |||
| public static final String FETCH_SHARD_STARTED = "fetch_shard_started"; | |||
| public static final String FETCH_SHARD_STORE = "fetch_shard_store"; | |||
| public static final String SYSTEM_READ = "system_read"; | |||
There was a problem hiding this comment.
nit: I like SEARCH_SYSTEM better but that's just a preference...
There was a problem hiding this comment.
Do you still like that since this also affects both search and get operations?
There was a problem hiding this comment.
Right, SYSTEM_READ sounds good then, thanks
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
jaymode
added a commit
to jaymode/elasticsearch
that referenced
this pull request
Aug 10, 2020
This commit introduces a new thread pool, `system_read`, which is intended for use by system indices for all read operations (get and search). The `system_read` pool is a fixed thread pool with a maximum number of threads equal to lesser of half of the available processors or 5. Given the combination of both get and read operations in this thread pool, the queue size has been set to 2000. The motivation for this change is to allow system read operations to be serviced in spite of the number of user searches. In order to avoid a significant performance hit due to pattern matching on all search requests, a new metadata flag is added to mark indices as system or non-system. Previously created system indices will have flag added to their metadata upon upgrade to a version with this capability. Additionally, this change also introduces a new class, `SystemIndices`, which encapsulates logic around system indices. Currently, the class provides a method to check if an index is a system index and a method to find a matching index descriptor given the name of an index. Relates elastic#50251 Relates elastic#37867
jaymode
added a commit
that referenced
this pull request
Aug 11, 2020
This commit introduces a new thread pool, `system_read`, which is intended for use by system indices for all read operations (get and search). The `system_read` pool is a fixed thread pool with a maximum number of threads equal to lesser of half of the available processors or 5. Given the combination of both get and read operations in this thread pool, the queue size has been set to 2000. The motivation for this change is to allow system read operations to be serviced in spite of the number of user searches. In order to avoid a significant performance hit due to pattern matching on all search requests, a new metadata flag is added to mark indices as system or non-system. Previously created system indices will have flag added to their metadata upon upgrade to a version with this capability. Additionally, this change also introduces a new class, `SystemIndices`, which encapsulates logic around system indices. Currently, the class provides a method to check if an index is a system index and a method to find a matching index descriptor given the name of an index. Relates #50251 Relates #37867 Backport of #57936
jaymode
added a commit
to jaymode/elasticsearch
that referenced
this pull request
Aug 11, 2020
This change adjusts the IndexMetadata serialization version for the system index flag after the backport of elastic#57936. Additionally, bwc tests have been reenabled.
jaymode
added a commit
that referenced
this pull request
Aug 11, 2020
This change adjusts the IndexMetadata serialization version for the system index flag after the backport of #57936. Additionally, bwc tests have been reenabled.
tlrx
added a commit
that referenced
this pull request
Sep 1, 2020
…m indices (#61785) The change #57936 introduced a dedicated thread pool for reads in system indices. It also introduced a potential NPE in the case the index to read in not yet present in the cluster state. This commit fixes that bug by using the getIndexSafe() instead of just index() method when retrieving the index's metadata so that an INFE is thrown if the index does not exist.
tlrx
added a commit
that referenced
this pull request
Sep 1, 2020
…m indices (#61785) (#61791) The change #57936 introduced a dedicated thread pool for reads in system indices. It also introduced a potential NPE in the case the index to read in not yet present in the cluster state. This commit fixes that bug by using the getIndexSafe() instead of just index() method when retrieving the index's metadata so that an INFE is thrown if the index does not exist.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces a new thread pool,
system_read, which isintended for use by system indices for all read operations (get and
search). The
system_readpool is a fixed thread pool with a maximumnumber of threads equal to lesser of half of the available processors
or 5. Given the combination of both get and read operations in this
thread pool, the queue size has been set to 2000. The motivation for
this change is to allow system read operations to be serviced in spite
of the number of user searches.
In order to avoid a significant performance hit due to pattern matching
on all search requests, a new metadata flag is added to mark indices
as system or non-system. Previously created system indices will have
flag added to their metadata upon upgrade to a version with this
capability.
Additionally, this change also introduces a new class,
SystemIndices,which encapsulates logic around system indices. Currently, the class
provides a method to check if an index is a system index and a method
to find a matching index descriptor given the name of an index.
Relates #50251
Relates #37867