Cut over to the Lucene filter cache #10897
Conversation
|
The PR is large so feel free to ping me if you would like to discuss it. |
| private Object readerCacheKey; | ||
|
|
||
| @Override | ||
| public Query rewrite(IndexReader reader) throws IOException { |
There was a problem hiding this comment.
Can this be final? If a subclass overrides this but does not call super, its caching will break.
There was a problem hiding this comment.
Some parent/child queries need to plug in other rewrite rules. I can try to make it final and add a protected doRewrite method for these queries to do what they need.
There was a problem hiding this comment.
I see. Not sure if its worth it, if doRewrite makes things complicated in its own way, given that createWeight is final and it will catch the issue.
|
look good. I pushed a few cleanups. I'm not happy with the complexity surrounding ShardCoreKeyMap etc: it is clearly not worth it. Related to that, I hit a test fail while playing: I think the 10 places where we do 'new IndexSearcher' should be reviewed and always have an explicit cache set |
|
@rmuir I pushed more commits to address your comments. I tend to agree with the complexity of ShardCoreKeyMap, it needs to be balanced with the benefit of having per-shard stats. The reason why I tried to hard to keep having per-shard stats is that doing otherwise would require an important refactoring of our monitoring APIs. |
|
Changes look great, +1 Yeah, as i said about the map, lets defer it to another issue. This one makes so much progress on its own. |
This removes Elasticsearch's filter cache and uses Lucene's instead. It has some
implications:
- custom cache keys (`_cache_key`) are unsupported
- decisions are made internally and can't be overridden by users ('_cache`)
- not only filters can be cached but also all queries that do not need scores
- parent/child queries can now be cached, however cached entries are only
valid for the current top-level reader so in practice it will likely only
be used on read-only indices
- the cache deduplicates filters, which plays nicer with large keys (eg. `terms`)
- better stats: we already had ram usage and evictions, but now also hit count,
miss count, lookup count, number of cached doc id sets and current number of
doc id sets in the cache
- dynamically changing the filter cache size is not supported anymore
Internally, an important change is that it removes the NoCacheFilter infrastructure
in favour of making Query.rewrite specializing the query for the current reader so
that it will only be cached on this reader (look for IndexCacheableQuery).
Note that consuming filters with the query API (createWeight/scorer) instead of
the filter API (getDocIdSet) is important for parent/child queries because
otherwise a QueryWrapperFilter(ParentQuery) would run the wrapped query per
segment while relations might be cross segments.
Core: Cut over to the Lucene filter cache. Close #10897
clintongormley
changed the title
clintongormley
added
the
:Search/Search
This removes Elasticsearch's filter cache and uses Lucene's instead. It has some
implications:
_cache_key) are unsupportedvalid for the current top-level reader so in practice it will likely only
be used in practice on read-only indices
terms)miss count, lookup count, number of cached doc id sets and current number of
doc id sets in the cache
Internally, an important change is that it removes the NoCacheFilter infrastructure
in favour of making Query.rewrite specializing the query for the current reader so
that it will only be cached on this reader (look for IndexCacheableQuery).
Note that consuming filters with the query API (createWeight/scorer) instead of
the filter API (getDocIdSet) is important for parent/child queries because
otherwise a QueryWrapperFilter(ParentQuery) would run the wrapped query per
segment while relations might be cross segments.