Introduce a `search_throttled` threadpool #33732

s1monw · 2018-09-15T07:36:49Z

Today all searches happen on the search threadpool which is the correct
behavior in almost any case. Yet, there are exceptions where for instance
searches searches should be passed through a single-thread
threadpool to reduce impact on a node. This change adds a index-private setting that allows to mark an index as throttled for searches and forks off all non-stats searcher access to this threadpool for indices that are marked as index.search.throttled

Today all searches happen on the search threadpool which is the correct behavior in almost any case. Yet, there are exceptions where for instance searches are required to succeed ie. in the case of a .security index. These searches should not be rejected if a node is under load. There are other more specialized usecases were searches should be passed through a single-thread threadpool to reduce impact on a node. Relates to elastic#33205

elasticmachine · 2018-09-15T07:36:51Z

Pinging @elastic/es-search-aggs

s1monw · 2018-09-15T07:38:04Z

the majority of the change is cutting SearchService over to async execution. The actual addition of the setting and such is really a smallish change.

tlrx

LGTM - I didn't expect this change to be so simple.

tlrx · 2018-09-17T08:46:42Z

server/src/main/java/org/elasticsearch/search/SearchService.java

-            cleanContext(context);
-        }
+    public void executeQueryPhase(InternalScrollSearchRequest request, SearchTask task,
+                                                     ActionListener<ScrollQuerySearchResult> listener) {


tlrx · 2018-09-17T08:47:29Z

server/src/main/java/org/elasticsearch/search/SearchService.java

+            this.canMatch = canMatch;
+        }
+
+


Nit: extra line

jpountz · 2018-09-17T15:08:53Z

I'm curious why we now fork can_match tasks unless the default threadpool is used, what is the benefit compared to never forking?

jasontedor · 2018-09-17T16:17:23Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java

+            }
+            if (ThreadPool.Names.SEARCH.equals(s) == false && ThreadPool.Names.GENERIC.equals(s) == false &&
+                ThreadPool.THREAD_POOL_TYPES.containsKey(s)) {
+                throw new IllegalArgumentException("Invalid valid for [index.search.threadpool] - " + s + " is a reserved built-in " +


valid -> value

jasontedor

I have some concerns.

jasontedor · 2018-09-17T16:24:24Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java

+            if (s == null || s.isEmpty()) {
+                throw new IllegalArgumentException("Value for [index.search.threadpool] must be a non-empty string.");
+            }
+            if (ThreadPool.Names.SEARCH.equals(s) == false && ThreadPool.Names.GENERIC.equals(s) == false &&


I really do not like that these can go on the generic thread pool. This thread pool can be quite large (we only recently bound it but it's a really large bound) and it has no queue. Especially since the need for this only arises if the node is under load, now that we are going unbounded it might only add to the pain the node is already experiencing. I understand the entire point is so that certain search requests never get rejected but I think we should find a different solution than using the generic thread pool for that. For example, could we force put them? Could we enable force putting them at the top of the queue?

s1monw · 2018-09-17T18:29:49Z

@jasontedor here is my suggestion.

I will add a search_single threadpool with a single thread and a queue.
users can either decide to go to search_single or search
in a seperate PR I add an option on the SearchRequest that does a force put on the queue that way we can fix Reduce the need to reload roles from index #33205 by special casing where necessary and not globally on the index.

@jpountz with 2. I think your question is more clear as well. for the search_single case we need to make sure nobody accesses more than one reader of an index that is marked as search_single at the same time.

I guess that makes this change more contained and addresses all concerns.

jasontedor · 2018-09-17T18:37:43Z

@s1monw The plan sounds good, I think that we are mostly on the same page. I do wonder if we need to allow users to use search_single AKA frozen, I think we can do it automatically for frozen indices and otherwise use search? Then this would not need to be user configurable at all?

s1monw · 2018-09-17T18:52:10Z

@s1monw The plan sounds good, I think that we are mostly on the same page. I do wonder if we need to allow users to use search_single AKA frozen, i think we can do it automatically for frozen indices and otherwise use search? Then this would not need to be user configurable at all?

I can do that. Private setting sounds good to me. then this PR will only add the setting and the reading. The test can add a plugin that allows changing it.

jasontedor · 2018-09-17T18:53:33Z

Thanks @s1monw.

jpountz · 2018-09-17T19:01:07Z

The plan sounds good to me too. Then maybe call the setting "frozen_search" rather than "search_single" since it would be dedicated to frozen indices?

s1monw · 2018-09-18T13:21:37Z

@jpountz @jasontedor I pushed a new commit implementing the first 2 steps. I still need to do some followups to ensure respect the search sequential in all places.

…ad if marked as such

s1monw · 2018-09-18T21:22:03Z

@elasticmachine test this please

s1monw · 2018-09-19T09:12:29Z

@jasontedor @jpountz I pushed more changes

jasontedor

Looks phenomenal @s1monw. Thank you for the extra iterations.

Today all searches happen on the search threadpool which is the correct behavior in almost any case. Yet, there are exceptions where for instance searches searches should be passed through a single-thread thread-pool to reduce impact on a node. This change adds a index-private setting that allows to mark an index as throttled for searches and forks off all non-stats searcher access to this thread-pool for indices that are marked as `index.search.throttled`

The executor was missed in the backport of #33732- Due to the internal forking to search or search_throttled threadpool there is no reason to fork to the search thread pool twice. Closes #37392 Relates to #33732

s1monw added >enhancement :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.5.0 labels Sep 15, 2018

s1monw requested review from jpountz, tlrx and bleskes September 15, 2018 07:36

tlrx approved these changes Sep 17, 2018

View reviewed changes

jasontedor reviewed Sep 17, 2018

View reviewed changes

jasontedor requested changes Sep 17, 2018

View reviewed changes

s1monw added 3 commits September 18, 2018 14:06

Merge branch 'master' into pluggable_search_thread_pool

5d8e2b9

cut over to a boolean setting

352e1b6

cut over to PrivateIndex settings

0b974f8

s1monw added 4 commits September 18, 2018 15:52

prepare validate action

a166ed2

Merge branch 'master' into pluggable_search_thread_pool

bfb6daa

Ensure all access to getSearcher executes on a search_sequential thre…

3f2fa47

…ad if marked as such

Fix imports

e447a2b

s1monw added 2 commits September 19, 2018 07:14

Merge branch 'master' into pluggable_search_thread_pool

1c39708

Rename search_sequentical to search_throttled and don't fork in canMatch

98a2aca

s1monw added 2 commits September 19, 2018 10:17

fix nits

dd3172b

fix test

5f8aa3b

jpountz approved these changes Sep 19, 2018

View reviewed changes

jasontedor approved these changes Sep 19, 2018

View reviewed changes

s1monw changed the title ~~Allow for pluggable search threadpool~~ Introduce a search_throttled threadpool Sep 20, 2018

s1monw merged commit 3522b90 into elastic:master Sep 20, 2018

s1monw added the backport pending label Sep 20, 2018

s1monw removed the backport pending label Sep 21, 2018

This was referenced Oct 8, 2018

Frozen Indices #34352

Closed

Prevent throttled indices to be searched through wildcards by default #34354

Merged

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

This was referenced Jan 13, 2019

Incorrect thread pool when registering Search related handlers to TransportService #37392

Closed

Use executor SAME to handle search related handlers #37427

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a `search_throttled` threadpool #33732

Introduce a `search_throttled` threadpool #33732

s1monw commented Sep 15, 2018 •

edited

elasticmachine commented Sep 15, 2018

s1monw commented Sep 15, 2018 •

edited

tlrx left a comment

tlrx Sep 17, 2018

tlrx Sep 17, 2018

jpountz commented Sep 17, 2018

jasontedor Sep 17, 2018

jasontedor left a comment

jasontedor Sep 17, 2018

s1monw commented Sep 17, 2018

jasontedor commented Sep 17, 2018 •

edited

s1monw commented Sep 17, 2018

jasontedor commented Sep 17, 2018

jpountz commented Sep 17, 2018

s1monw commented Sep 18, 2018

s1monw commented Sep 18, 2018

s1monw commented Sep 19, 2018

jasontedor left a comment

Introduce a search_throttled threadpool #33732

Introduce a search_throttled threadpool #33732

Conversation

s1monw commented Sep 15, 2018 • edited

elasticmachine commented Sep 15, 2018

s1monw commented Sep 15, 2018 • edited

tlrx left a comment

Choose a reason for hiding this comment

tlrx Sep 17, 2018

Choose a reason for hiding this comment

tlrx Sep 17, 2018

Choose a reason for hiding this comment

jpountz commented Sep 17, 2018

jasontedor Sep 17, 2018

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Sep 17, 2018

Choose a reason for hiding this comment

s1monw commented Sep 17, 2018

jasontedor commented Sep 17, 2018 • edited

s1monw commented Sep 17, 2018

jasontedor commented Sep 17, 2018

jpountz commented Sep 17, 2018

s1monw commented Sep 18, 2018

s1monw commented Sep 18, 2018

s1monw commented Sep 19, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Introduce a `search_throttled` threadpool #33732

Introduce a `search_throttled` threadpool #33732

s1monw commented Sep 15, 2018 •

edited

s1monw commented Sep 15, 2018 •

edited

jasontedor commented Sep 17, 2018 •

edited