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

Query Cache: Support shard level query response caching #7161

Closed
wants to merge 4 commits into from

Conversation

kimchy
Copy link
Member

@kimchy kimchy commented Aug 5, 2014

The query cache allow to cache the (binary serialized) response of the shard level query phase execution based on the actual request as the key. The cache is fully coherent with the semantics of NRT, with a refresh (that actually ended up refreshing) causing previous cached entries on the relevant shard to be invalidated and eventually evicted.

This change enables query caching as an opt in index level setting, called index.cache.query.enable and defaults to false. The setting can be changed dynamically on an index. The cache is only enabled for search requests with search_type count.

The indices query cache is a node level query cache. The indices.cache.query.size controls what is the size (bytes wise) the cache will take, and defaults to 1% of the heap. Note, this cache is very effective with small values in it already. There is also the advanced option to set indices.cache.query.expire that allow to control after a certain time of inaccessibility the cache will be evicted.

Note, the request takes the search "body" as is (bytes), and uses it as the key. This means same JSON but with different key order will constitute different cache entries.

This change includes basic stats (shard level, index/indices level, and node level) for the query cache, showing how much is used and eviction rates.

While this is a good first step, and the goal is to get it in, there are a few things that would be great additions to this work, but they can be done as additional pull requests:

  • More stats, specifically cache hit and cache miss, per shard.
  • Request level flag, defaults to "not set" (inheriting what the setting is).
  • Allowing to change the cache size using the cluster update settings API
  • Consider enabling the cache to query phase also when asking hits are involved, note, this will only include the "top docs", not the actual hits.
  • See if there is a performant manner to solve the "out of order" of keys in the JSON case.
  • Maybe introduce a filter element, that is outside of the request, that is checked, and if it matches all docs in a shard, will not be used as part of the key. This will help with time based indices and moving windows for shards that fall "inside" the window to be more effective caching wise.
  • Add a more infra level support in search context that allows for any element to mark the search as non deterministic (on top of the support for "now"), and use it to not cache search responses.


@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(Fields.FilterCacheStats);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/FilterCache/QueryCache/ ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aye, will change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aye, will change


private class CleanupKey implements IndexReader.ReaderClosedListener {
IndexShard indexShard;
long readerVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining why you use the long version as opposed to the cache&delete key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@jpountz
Copy link
Contributor

jpountz commented Aug 5, 2014

I left some minor comments on the PR but overall it looks good.

I would add an item to the TODO list that looks important to me: a more generic ability to disable the query cache from the various query/filter parsers so that the random function score could prevent the query from being cached when there is no explicit seed.

@jpountz jpountz removed the review label Aug 5, 2014
The query cache allow to cache the (binary serialized) response of the shard level query phase execution based on the actual request as the key. The cache is fully coherent with the semantics of NRT, with a refresh (that actually ended up refreshing) causing previous cached entries on the relevant shard to be invalidated and eventually evicted.

This change enables query caching as an opt in index level setting, called `index.cache.query.enable` and defaults to `false`. The setting can be changed dynamically on an index. The cache is only enabled for search requests with search_type count.

The indices query cache is a node level query cache. The `indices.cache.query.size` controls what is the size (bytes wise) the cache will take, and defaults to `1%` of the heap. Note, this cache is very effective with small values in it already. There is also the advanced option to set `indices.cache.query.expire` that allow to control after a certain time of inaccessibility the cache will be evicted.

Note, the request takes the search "body" as is (bytes), and uses it as the key. This means same JSON but with different key order will constitute different cache entries.

This change includes basic stats (shard level, index/indices level, and node level) for the query cache, showing how much is used and eviction rates.

While this is a good first step, and the goal is to get it in, there are a few things that would be great additions to this work, but they can be done as additional pull requests:

- More stats, specifically cache hit and cache miss, per shard.
- Request level flag, defaults to "not set" (inheriting what the setting is).
- Allowing to change the cache size using the cluster update settings API
- Consider enabling the cache to query phase also when asking hits are involved, note, this will only include the "top docs", not the actual hits.
- See if there is a performant manner to solve the "out of order" of keys in the JSON case.
- Maybe introduce a filter element, that is outside of the request, that is checked, and if it matches all docs in a shard, will not be used as part of the key. This will help with time based indices and moving windows for shards that fall "inside" the window to be more effective caching wise.
- Add a more infra level support in search context that allows for any element to mark the search as non deterministic (on top of the support for "now"), and use it to not cache search responses.

closes elastic#7161
@kimchy
Copy link
Member Author

kimchy commented Aug 5, 2014

@jpountz addressed your comments, ready for another round...

@kimchy kimchy added the review label Aug 5, 2014
@clintongormley
Copy link

If the query cache already takes refreshes into account, why do we need the expire setting?

See if there is a performant manner to solve the "out of order" of keys in the JSON case.

This could also be handled client side, ie: if a cache flag is passed, then we generate "canonical" JSON (ie keys are emitted in sorted order)

@kimchy
Copy link
Member Author

kimchy commented Aug 5, 2014

If the query cache already takes refreshes into account, why do we need the expire setting?

I don't see a big use case for it, just for completeness sake to be honest. Imagine an index that doesn't change, but still wanting to expire based on time for some reason, and not just based on size.

super(settings);
this.clusterService = clusterService;
this.threadPool = threadPool;
this.cleanInterval = componentSettings.getAsTime("clean_interval", TimeValue.timeValueSeconds(60));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a static string for the full-qualified setting? I think we discussed moving away from component settings, and using the full string makes the source code much more grep-able.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add

@dakrone
Copy link
Member

dakrone commented Aug 5, 2014

I think we should add to the TODOs returning a key in the response whether the response was served from the cache or not, something like "cache_hit": true. It makes 3rd-party tracking of cache hits/misses easier.

@kimchy
Copy link
Member Author

kimchy commented Aug 5, 2014

@dakrone agreed, that would be nice as well (and it needs to be on the shard level element, btw, so I would opt for only setting it if its there)

}

static final class Fields {
static final XContentBuilderString QueryCacheStats = new XContentBuilderString("query_cache");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should be in upper case as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change

@jpountz
Copy link
Contributor

jpountz commented Aug 5, 2014

LGTM

* since we are checking on the cluster state IndexMetaData always.
*/
public static final String INDEX_CACHE_QUERY_ENABLED = "index.cache.query.enable";
public static final String INDEX_CACHE_QUERY_CLEAN_INTERVAL = "index.cache.query.clean_interval";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be "indices" instead of "index" since this is for all indices instead of a single index-level setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!, will fix

@dakrone
Copy link
Member

dakrone commented Aug 5, 2014

@kimchy left one comment about the clean_interval setting name, other than that LGTM.

@kimchy kimchy closed this in 418ce50 Aug 5, 2014
kimchy added a commit that referenced this pull request Aug 5, 2014
The query cache allow to cache the (binary serialized) response of the shard level query phase execution based on the actual request as the key. The cache is fully coherent with the semantics of NRT, with a refresh (that actually ended up refreshing) causing previous cached entries on the relevant shard to be invalidated and eventually evicted.

This change enables query caching as an opt in index level setting, called `index.cache.query.enable` and defaults to `false`. The setting can be changed dynamically on an index. The cache is only enabled for search requests with search_type count.

The indices query cache is a node level query cache. The `indices.cache.query.size` controls what is the size (bytes wise) the cache will take, and defaults to `1%` of the heap. Note, this cache is very effective with small values in it already. There is also the advanced option to set `indices.cache.query.expire` that allow to control after a certain time of inaccessibility the cache will be evicted.

Note, the request takes the search "body" as is (bytes), and uses it as the key. This means same JSON but with different key order will constitute different cache entries.

This change includes basic stats (shard level, index/indices level, and node level) for the query cache, showing how much is used and eviction rates.

While this is a good first step, and the goal is to get it in, there are a few things that would be great additions to this work, but they can be done as additional pull requests:

- More stats, specifically cache hit and cache miss, per shard.
- Request level flag, defaults to "not set" (inheriting what the setting is).
- Allowing to change the cache size using the cluster update settings API
- Consider enabling the cache to query phase also when asking hits are involved, note, this will only include the "top docs", not the actual hits.
- See if there is a performant manner to solve the "out of order" of keys in the JSON case.
- Maybe introduce a filter element, that is outside of the request, that is checked, and if it matches all docs in a shard, will not be used as part of the key. This will help with time based indices and moving windows for shards that fall "inside" the window to be more effective caching wise.
- Add a more infra level support in search context that allows for any element to mark the search as non deterministic (on top of the support for "now"), and use it to not cache search responses.

closes #7161
@kimchy kimchy deleted the query_cache branch August 5, 2014 15:49
clintongormley added a commit that referenced this pull request Aug 6, 2014
clintongormley added a commit that referenced this pull request Aug 6, 2014
clintongormley added a commit that referenced this pull request Aug 6, 2014
clintongormley added a commit that referenced this pull request Aug 6, 2014
@jpountz jpountz removed the review label Aug 11, 2014
kimchy added a commit that referenced this pull request Sep 8, 2014
The query cache allow to cache the (binary serialized) response of the shard level query phase execution based on the actual request as the key. The cache is fully coherent with the semantics of NRT, with a refresh (that actually ended up refreshing) causing previous cached entries on the relevant shard to be invalidated and eventually evicted.

This change enables query caching as an opt in index level setting, called `index.cache.query.enable` and defaults to `false`. The setting can be changed dynamically on an index. The cache is only enabled for search requests with search_type count.

The indices query cache is a node level query cache. The `indices.cache.query.size` controls what is the size (bytes wise) the cache will take, and defaults to `1%` of the heap. Note, this cache is very effective with small values in it already. There is also the advanced option to set `indices.cache.query.expire` that allow to control after a certain time of inaccessibility the cache will be evicted.

Note, the request takes the search "body" as is (bytes), and uses it as the key. This means same JSON but with different key order will constitute different cache entries.

This change includes basic stats (shard level, index/indices level, and node level) for the query cache, showing how much is used and eviction rates.

While this is a good first step, and the goal is to get it in, there are a few things that would be great additions to this work, but they can be done as additional pull requests:

- More stats, specifically cache hit and cache miss, per shard.
- Request level flag, defaults to "not set" (inheriting what the setting is).
- Allowing to change the cache size using the cluster update settings API
- Consider enabling the cache to query phase also when asking hits are involved, note, this will only include the "top docs", not the actual hits.
- See if there is a performant manner to solve the "out of order" of keys in the JSON case.
- Maybe introduce a filter element, that is outside of the request, that is checked, and if it matches all docs in a shard, will not be used as part of the key. This will help with time based indices and moving windows for shards that fall "inside" the window to be more effective caching wise.
- Add a more infra level support in search context that allows for any element to mark the search as non deterministic (on top of the support for "now"), and use it to not cache search responses.

closes #7161
clintongormley added a commit that referenced this pull request Sep 8, 2014
clintongormley added a commit that referenced this pull request Sep 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants