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

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.
@kimchy kimchy added review labels Aug 5, 2014
@jpountz
jpountz reviewed Aug 5, 2014
View changes
src/main/java/org/elasticsearch/index/cache/query/QueryCacheStats.java Outdated

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

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

s/FilterCache/QueryCache/ ?

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

aye, will change

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

aye, will change

@jpountz
jpountz reviewed Aug 5, 2014
View changes
src/main/java/org/elasticsearch/index/cache/query/QueryCacheStats.java Outdated
}

static final class Fields {
static final XContentBuilderString FilterCacheStats = new XContentBuilderString("query_cache");

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

s/FilterCache/QueryCache/

@jpountz
jpountz reviewed Aug 5, 2014
View changes
src/main/java/org/elasticsearch/indices/cache/query/IndicesQueryCache.java Outdated

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

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

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

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

will do

@jpountz
jpountz reviewed Aug 5, 2014
View changes
src/main/java/org/elasticsearch/indices/cache/query/IndicesQueryCache.java Outdated
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeLong(id);
// shardTarget.writeTo(out); not needed

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

can you fix indentation

@jpountz
jpountz reviewed Aug 5, 2014
View changes
src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java Outdated
@@ -442,6 +443,10 @@ protected boolean randomizeNumberOfShardsAndReplicas() {
// Randomly load or don't load bloom filters:
builder.put(CodecService.INDEX_CODEC_BLOOM_LOAD, random.nextBoolean());

if (random.nextBoolean()) {
builder.put(IndicesQueryCache.INDEX_QUERY_CACHE_ENABLED, random.nextBoolean());
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

nice

private final TimeValue cleanInterval;
private final Reaper reaper;

final ConcurrentMap<CleanupKey, Boolean> registeredClosedListeners = ConcurrentCollections.newConcurrentMap();

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

ConcurrentCollections.newConcurrentSet instead? the value doesn't seem useful?

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

I need the putIfAbsent in ConcurrentMap...

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

Hmm, I'm not sure why you really need it? Why wouldn't it work to replace:

ConcurrentMap<CleanupKey, Boolean> registeredClosedListeners = ...;
if (!registeredClosedListeners.containsKey(cleanupKey)) {
  Boolean previous = registeredClosedListeners.putIfAbsent(cleanupKey, Boolean.TRUE);
  if (previous == null) {
    context.searcher().getIndexReader().addReaderClosedListener(cleanupKey);
  }
}

with

Set<CleanupKey> registeredClosedListeners = ...;
if (registeredClosedListeners.add(cleanupKey)) {
  context.searcher().getIndexReader().addReaderClosedListener(cleanupKey);
}

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

Here is the problem, we are using a Set backed map for the concurrentMap, wrapping it with CHM. The problem is that a Set backed map in Java, see Collections#SetFromMap implements add as follows: return m.put(e, Boolean.TRUE) == null;. This means that add will do a put each time and replaces the key, which is much more expensive on a CHM compared to doing a get (and I don't want to key to be replaced), and then if there is a miss, do a putIfAbsent. Even putIfAbsent is more expensive, and the most common case is we already registered that listener, so much better to do the lightweight read first.

@jpountz
jpountz reviewed Aug 5, 2014
View changes
src/main/java/org/elasticsearch/indices/cache/query/IndicesQueryCache.java Outdated
* similar requests that are potentially expensive (because of aggs for example). The cache is fully coherent
* with the semantics of NRT (the index reader version is part of the cache key), and relies on size based
* eviction to evict old reader associated cache entries (which seems to be ok, no need to complicate the
* logic with traversing eviction entries).

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

That comment looks outdated?

@jpountz

This comment has been minimized.

Copy link
Contributor

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 #7161
@kimchy

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2014

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

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

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

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

This comment has been minimized.

Copy link
Member Author

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));

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

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.

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

will add

this.threadPool = threadPool;
this.cleanInterval = componentSettings.getAsTime("clean_interval", TimeValue.timeValueSeconds(60));
// this cache can be very small yet still be very effective
this.size = settings.get(INDICES_CACHE_QUERY_SIZE, "1%");

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

We have settings.getAsMemory now, so we can replace the MemorySizeValue.parseBytesSizeValueOrHeapRatio(size).bytes(); in buildCache() below.

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

we can't because we need to keep the string size around for the guava limit mentioned in the buildCache comment

return false;
}
if (context.nowInMillisUsed()) {
return false;

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

Can you add a comment why we can't cache the request if nowInMillisUsed() is true?

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

will comment

assert canCache(request, context);
Key key = buildKey(request, context);
Loader loader = new Loader(queryPhase, context, key);
BytesReference value = cache.get(key, loader);

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

This is very highlevel, but what do you think about following the fielddata pattern of .load() and .loadDirect() for this? I think it might be nice to standardize even if it's not through a single interface.

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

I don't think its applicable here?

private SearchShardTarget shardTarget;
private BytesReference data;

private transient QuerySearchResult result;

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

I thought transient was for Java's built-in serialization, what does this transient marker do here?

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

I view it as documentation element, even if we don't use Java serialization, it shows that this field will not get serialized.

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

Okay, that works for me, just wanted make sure there wasn't an obscure javaism that I had missed :)


/**
* this class aim is to just provide an on the write *write* format that is the same as {@link QuerySearchResult}
* and also provide a nice wrapper for in node communication.

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

This javadoc doesn't make much sense to me maybe change this class aim is to just provide an on the write *write* format that is the same as {@link QuerySearchResult} to this class' aim is to provide the write format that is the same as {@link QuerySearchResult}? (if that's what it means, it's confusing to me)

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

it should be "on the wire", will make it more clear

@dakrone

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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");

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 5, 2014

Contributor

this one should be in upper case as well?

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

will change

kimchy added 2 commits Aug 5, 2014
@jpountz

This comment has been minimized.

Copy link
Contributor

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";

This comment has been minimized.

Copy link
@dakrone

dakrone Aug 5, 2014

Member

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

This comment has been minimized.

Copy link
@kimchy

kimchy Aug 5, 2014

Author Member

agreed!, will fix

@dakrone

This comment has been minimized.

Copy link
Member

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 kimchy:query_cache branch Aug 5, 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
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
4 participants
You can’t perform that action at this time.