Automatically adjust pre_filter_shard_size to 1 for readonly indices #43377
Conversation
Now that we split the search execution in two whenever searching read-only and write indices as part of the same request (see elastic#42510), we can also automatically set `pre_filter_shard_size` to the appropriate value whenever not explicitly provided: `1` for readonly indices, and `128` (like before this change) for write indices. Note that we may still end up searching write and readonly indices as part of the same search execution, for instance when a scroll is provided or size is set to `0`, in which case we set `pre_filter_shard_size` to `128` when not explicitly set. Closes elastic#39835
javanna
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search |
| The default value for `pre_filter_shard_size` is `128` but it's recommended to set it to `1` when searching frozen indices. There is no | ||
| significant overhead associated with this pre-filter phase. | ||
| a threshold that, when exceeded, will enforce a round-trip to pre-filter search shards that cannot possibly match. Whenever not explicitly | ||
| set, the parameter is automatically adjusted to `1` for read-only indices, and to `128 for write indices. This filter phase can limit the |
There was a problem hiding this comment.
nit: missing "`" after 128.
| @@ -60,7 +60,7 @@ | |||
|
|
|||
| private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false")); | |||
|
|
|||
| public static final int DEFAULT_PRE_FILTER_SHARD_SIZE = 128; | |||
| public static final int DEFAULT_PRE_FILTER_SHARD_SIZE = -1; | |||
There was a problem hiding this comment.
nit: we serialize this value using read/writeVInt so -1 is not a very good default since it will always use 5 bytes. This shouldn't affect anything but we also disallow setting this value to a negative number explicitly in SearchRequest so it will not be possible for users to restore the default value in a SearchRequest. I wonder if it would be simpler to use an Integer and readOptionaVInt to make the intent clear ?
There was a problem hiding this comment.
good point: I follow the bytes reasoning more than the resetting issue, cause generally we do not allow null values either when validating requests, hence you need to create a new request to go back to the default values. I am not a big fan of null values, so I have a slight preference for -1 but I can also change that
|
run elasticsearch-ci/1 |
|
Hi @javanna, could you please give a quick update on what's the status of this PR? Also to one of your questions above:
I would totally support that suggestion of moving this to a lower level. Kibana uses |
|
heya @timroes as for the status of this PR, it has stalled because we were not too happy with the limitations it has. We were discussing as an alternative to potentially apply 1 and force the execution of the can_match phase for every search no matter which indices it executes against. I need to sync with the team on where we stand with that change. In the meantime I will close this PR as it will not go in as-is for the reason stated above. |
|
Some more context: we no longer split the execution in two when readonly and non-readonly indices are searched at the same time (see #49471), hence this change is not even possible at this level, yet another reason to close this PR and consider a different approach. |
Now that we split the search execution in two whenever searching read-only and
write indices as part of the same request (see #42510), we can also automatically
set
pre_filter_shard_sizeto the appropriate value whenever not explicitlyprovided:
1for readonly indices, and128(like before this change) for writeindices.
Note that we may still end up searching write and readonly indices as part of the
same search execution, for instance when a scroll is provided or size is set to
0,in which case we set
pre_filter_shard_sizeto128when not explicitly set.Open questions:
pre_filter_shard_sizeis not set: it is not disruptive, yetdoes it deserve an entry in the migrate guide? If so we may want to discuss whether we can
backport this change or not to 7.x.
1if we are searching read-only indices alone, or if we are splitting the search execution. There are cases (for instance whensizeis set to0) where we don't split the search execution, yet readonly indices will be searched as part of the same execution as write indices, in which case they will get128as a default value. Maybe we would like to apply the setting at a lower level so that when size is0we still apply1to read-only indices?Closes #39835