Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request cache and Runtime Fields #62033

Closed
jimczi opened this issue Sep 7, 2020 · 1 comment · Fixed by #66295
Closed

Request cache and Runtime Fields #62033

jimczi opened this issue Sep 7, 2020 · 1 comment · Fixed by #66295
Assignees
Labels
blocker >bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.11.0 v7.12.0

Comments

@jimczi
Copy link
Contributor

jimczi commented Sep 7, 2020

Modifications of Runtime Fields in the mapping should invalidate the request cache. Currently, search requests that target runtime fields are eligible to the request cache but they are not removed when a modification occurs. We consider this behavior as a non-released bug.
We discussed several solutions to this problem:

  • Add a serializable mapping version in the shard search request cache key.

We already expose a mapping version that is incremented on each mapping update. However we don't force a single mapping version per request. The mapping service can be updated in the middle of the parsing of a search request so multiple versions of the mapping can co-exist in a single shard search request.
We could force a single version during the parsing but that would require to add a way to clone a mapping service so that's not a low hanging fruit.

  • Add versioning for runtime fields that is plugged in the shard search request cache key.

With one version per field we could associate each runtime field used in the search request with its version.
Although, extracting all fields that appear in a search request is not trivial. We don't have visitors for aggregations or query builders so that would require adding one. The advantage of this option is that caching would be invalidated only for search requests that target a modified runtime field.

  • Invalidate all request cache entries whenever there is an update in the mapping.

Updating the mapping requires a cluster update so this event should be rare even with runtime fields.
We have plans to add the ability to plug runtime fields on top of the mapping (in the search request or in datastreams)
in order to add more flexibility. For these reasons, a simple solution could be to invalidate the shard request cache entries whenever there is an update in the mapping. We already have this invalidation when the searcher reopens so that should be straightforward to add.

@jimczi jimczi added >bug blocker :Search/Search Search-related issues that do not fall into other categories v7.10.0 labels Sep 7, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Sep 7, 2020
@javanna javanna added v7.11.0 and removed v7.10.0 labels Oct 7, 2020
@nik9000 nik9000 self-assigned this Oct 27, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Nov 24, 2020
This makes sure that we only serve a hit from the request cache if it
was build using the same mapping and that the entire search phase is
processed using the same mapping. These are both bugs that Elasticsearch
has had for a long, long time but now that we have runtime fields they
are much more likely to matter because runtime fields can be changed
*drastically* from moment to moment.

It does by adding a counter to the local mapping that is incremented
whenever the mapping changes. This counter and the mapping is stored
under a single `volatile` variable that is updated to point to the new
mapping whenever the mapping changes. When the search phase begins we
read the contents of the `volatile` and use it to power the entire
phase, and including the version counter into the cache key.

Before this change we'd go back to the `volatile` over and over again,
potentially getting a new version of the mapping every time. This was
convenient but made it possible that we'd see the mapping update half
way through the search phase which could cause trouble.

Mechanically, this creates a `snapshot()` method on `MapperService` to
do the volatile read of the current mapping. The snapshot does all of
the reading we need from here on out. I kept all of the methods on
`MapperService` that read directly from the mapping but deprecated all
of them because using them can lead to seeing the mapping updates when
you don't expect them. We can slowly remove the usages in follow up
changes. This change is big enough as is.

Sadly, the `Mapper`s are also mutable. Luckilly, we only mutate them
before sticking them behind the `volatile` reference. Hopefully. There
is a lot of code there and it is hard to be sure. But it looks like we
*intend* to do this. We should make them properly immutable in a follow
up change.

Closes elastic#62033
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Dec 23, 2020
…66295)

This makes sure that we only serve a hit from the request cache if it
was build using the same mapping and that the same mapping is used for
the entire "query phase" of the search.

Closes elastic#62033
nik9000 added a commit that referenced this issue Dec 23, 2020
This makes sure that we only serve a hit from the request cache if it
was build using the same mapping and that the same mapping is used for
the entire "query phase" of the search.

Closes #62033
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Dec 23, 2020
…66295)

This makes sure that we only serve a hit from the request cache if it
was build using the same mapping and that the same mapping is used for
the entire "query phase" of the search.

Closes elastic#62033
nik9000 added a commit that referenced this issue Dec 23, 2020
…66803)

This makes sure that we only serve a hit from the request cache if it
was build using the same mapping and that the same mapping is used for
the entire "query phase" of the search.

Closes #62033
nik9000 added a commit that referenced this issue Dec 23, 2020
…66795)

This makes sure that we only serve a hit from the request cache if it
was build using the same mapping and that the same mapping is used for
the entire "query phase" of the search.

Closes #62033
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker >bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.11.0 v7.12.0
Projects
None yet
5 participants