Scrolls - failing on partial results

[markharwood]

A bug in the existing impl

In #27435 we introduced an "allow partial search results" flag which if set to false should return errors rather than hits. This currently works on the first request to the myindex/_search?scroll=1m api but fails to fail on the subsequent /_search/scroll calls. This is a bug caused by the fact that SearchScrollAsyncAction is missing the logic that was added to regular AbstractSearchAsynction for regular searches.

Some questions about scroll in general

Before we fix this there are a number of important questions that need answering about the behaviour of these flags:

1. Should only the first setup-request for scrolls accept the allowPartialSearchResults parameter? (I think "yes" - there's no good reason to change policy in subsequent calls, mid-scroll).
2. Should the default cluster setting for allowPartialSearchResults also apply to scrolls? (@jimczi thinks not - in his view a good default for regular searches is to serve partials, while scrolls should not)
3. If the answer to 2) is "no" do we use a different flag name for scrolls to avoid any confusion e.g. allowPartialScrollResults?

Note that these questions are related to the discussion on flag defaults for regular search on #28494 - if we opt for a strict default on searches there then @jimczi's concerns over defaulting scrolls to strict are met and both search and scroll can share a common cluster default setting.

@jpountz and @colings86 - you may have opinions

[jpountz]

Should only the first setup-request for scrolls accept the allowPartialSearchResults parameter? (I think "yes" - there's no good reason to change policy in subsequent calls, mid-scroll).

Agreed.

Should the default cluster setting for allowPartialSearchResults also apply to scrolls? (@jimczi thinks not - in his view a good default for regular searches is to serve partials, while scrolls should not)

Tricky question. I understand the reasoning but at the same time I don't like introducing exceptions. Let's wait for a resolution on #28494? Like you said if we agree to change the default in 7.0 to false, then we don't need to treat scrolls differently. Until then, we can make the default apply to scroll requests as well, this is no different from what users have experienced before.

[markharwood]

We discussed this on Fix-it-Friday and decided that what we should do starting in 6.3 is to make the scroll behaviour obey the choice of allowPartialSearchResults setting and encourage people to experiment with turning OFF the default of allowing partial results.
If folks like the behaviour of errors rather than partial results then we are in a good place. However, if users decide they generally still want partial search results but errors on scrolls we may need to look at introducing 2 separate flags for controlling scroll and search behaviours.

[cbuescher]

@elastic/es-search-aggs looks like this has been discussed and @markharwood is tracking it

[colings86]

We should ensure the solution takes into account timeouts as well as shard failures on scroll requests (see #16555 for context)