Added soft limit to open scroll contexts #25244 #36009
Conversation
jimczi
added
>feature
:Search/Search
|
Pinging @elastic/es-search |
| @@ -696,6 +710,7 @@ public boolean freeContext(long id) { | |||
| assert context.refCount() > 0 : " refCount must be > 0: " + context.refCount(); | |||
| context.indexShard().getSearchOperationListener().onFreeContext(context); | |||
| if (context.scrollContext() != null) { | |||
| openScrollContexts.decrementAndGet(); | |||
There was a problem hiding this comment.
we should also check the scroll contexts in freeAllContextForIndex above ?
There was a problem hiding this comment.
Doesn't freeAllContextForIndex simply call freeContext ? Am I supposed to do any extra processing here?
There was a problem hiding this comment.
Sorry my bad, it does call freeContext
| @@ -145,6 +146,9 @@ | |||
| public static final Setting<Boolean> DEFAULT_ALLOW_PARTIAL_SEARCH_RESULTS = | |||
| Setting.boolSetting("search.default_allow_partial_results", true, Property.Dynamic, Property.NodeScope); | |||
|
|
|||
| public static final Setting<Integer> MAX_OPEN_SCROLL_CONTEXT = | |||
| Setting.intSetting("search.max_open_scroll_context", 150, 0, Property.NodeScope); | |||
There was a problem hiding this comment.
This setting should be dynamic, you should extract the value in the SearchService ctr and register an update consumer. DEFAULT_ALLOW_PARTIAL_SEARCH_RESULTS is a good example of how to do it.
There was a problem hiding this comment.
Should be dynamic now. Please review the changes.
| @@ -145,6 +146,9 @@ | |||
| public static final Setting<Boolean> DEFAULT_ALLOW_PARTIAL_SEARCH_RESULTS = | |||
| Setting.boolSetting("search.default_allow_partial_results", true, Property.Dynamic, Property.NodeScope); | |||
|
|
|||
| public static final Setting<Integer> MAX_OPEN_SCROLL_CONTEXT = | |||
| Setting.intSetting("search.max_open_scroll_context", 150, 0, Property.Dynamic, Property.NodeScope); | |||
There was a problem hiding this comment.
Let's start at 500 ? That's the number we agreed on in #29252.
There was a problem hiding this comment.
Sure, i'll change it while I update the docs
|
@elasticmachine test this please |
|
@elasticmachine test this please |
This change adds a soft limit to open scroll contexts that can be controlled with the dynamic cluster setting `search.max_open_scroll_context` (defaults to 500).
Missing updated documentation. Will be added later.
Since development had already begun on this issue before the suggestions made by @geekpete , we thought it would be best to first make a pull request for the initial situation.
Closes #25244