From 364c110748e73db3f00209bf49a8c0565539c71b Mon Sep 17 00:00:00 2001 From: jimczi Date: Thu, 17 Oct 2019 17:03:59 +0200 Subject: [PATCH 1/4] Disable caching when queries are profiled This change disables the query and request cache when profile is set to true in the request. This means that profiled queries will not check caches to execute the query and the result will never be added in the cache either. Closes #33298 --- .../elasticsearch/indices/IndicesService.java | 5 +++ .../search/internal/ContextIndexSearcher.java | 17 +++++++ .../search/profile/query/ProfileWeight.java | 2 +- .../indices/IndicesRequestCacheIT.java | 45 ++++++++++++++++++- .../profile/query/QueryProfilerTests.java | 31 +++++-------- 5 files changed, 78 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/IndicesService.java b/server/src/main/java/org/elasticsearch/indices/IndicesService.java index 2000467f5c1e2..8ac5682405568 100644 --- a/server/src/main/java/org/elasticsearch/indices/IndicesService.java +++ b/server/src/main/java/org/elasticsearch/indices/IndicesService.java @@ -1246,6 +1246,11 @@ public boolean canCache(ShardSearchRequest request, SearchContext context) { return false; } + // Profiled queries should not use the cache + if (request.source() != null && request.source().profile()) { + return false; + } + IndexSettings settings = context.indexShard().indexSettings(); // if not explicitly set in the request, use the index setting, if not, use the request if (request.requestCache() == null) { diff --git a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java index e2b8e8ab7afd6..33c045605c938 100644 --- a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java @@ -67,6 +67,19 @@ public class ContextIndexSearcher extends IndexSearcher { */ private static int CHECK_CANCELLED_SCORER_INTERVAL = 1 << 11; + /** + * A policy to bypass the cache entirely + */ + private static final QueryCachingPolicy NEVER_CACHE_POLICY = new QueryCachingPolicy() { + @Override + public void onUse(Query query) {} + + @Override + public boolean shouldCache(Query query) { + return false; + } + }; + private AggregatedDfs aggregatedDfs; private QueryProfiler profiler; private Runnable checkCancelled; @@ -79,6 +92,10 @@ public ContextIndexSearcher(IndexReader reader, Similarity similarity, QueryCach } public void setProfiler(QueryProfiler profiler) { + if (profiler != null) { + // disable query caching on profiled query + setQueryCachingPolicy(NEVER_CACHE_POLICY); + } this.profiler = profiler; } diff --git a/server/src/main/java/org/elasticsearch/search/profile/query/ProfileWeight.java b/server/src/main/java/org/elasticsearch/search/profile/query/ProfileWeight.java index ae4481949ffd1..8e1d4ed133b45 100644 --- a/server/src/main/java/org/elasticsearch/search/profile/query/ProfileWeight.java +++ b/server/src/main/java/org/elasticsearch/search/profile/query/ProfileWeight.java @@ -120,7 +120,7 @@ public void extractTerms(Set set) { @Override public boolean isCacheable(LeafReaderContext ctx) { - return subQueryWeight.isCacheable(ctx); + return false; } } diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java index 68653223cf554..a3a8436f822ba 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java @@ -29,6 +29,7 @@ import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.index.cache.request.RequestCacheStats; import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.index.query.TermQueryBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket; @@ -414,9 +415,49 @@ public void testCacheWithFilteredAlias() { assertCacheState(client, "index", 2, 2); } + public void testProfileDisableCache() throws Exception { + Client client = client(); + assertAcked( + client.admin().indices().prepareCreate("index") + .addMapping("_doc", "k", "type=keyword") + .setSettings( + Settings.builder() + .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) + .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) + ) + .get() + ); + indexRandom(true, client.prepareIndex("index", "_doc").setSource("k", "hello")); + ensureSearchable("index"); + + int expectedHits = 0; + int expectedMisses = 0; + for (int i = 0; i < 5; i++) { + boolean profile = i % 2 == 0; + SearchResponse resp = client.prepareSearch("index") + .setRequestCache(true) + .setProfile(profile) + .setQuery(QueryBuilders.termQuery("k", "hello")) + .get(); + assertSearchResponse(resp); + ElasticsearchAssertions.assertAllSuccessful(resp); + assertThat(resp.getHits().getTotalHits().value, equalTo(1L)); + if (profile == false) { + if (i == 1) { + expectedMisses ++; + } else { + expectedHits ++; + } + } + assertCacheState(client, "index", expectedHits, expectedMisses); + } + } + private static void assertCacheState(Client client, String index, long expectedHits, long expectedMisses) { - RequestCacheStats requestCacheStats = client.admin().indices().prepareStats(index).setRequestCache(true).get().getTotal() - .getRequestCache(); + RequestCacheStats requestCacheStats = client.admin().indices().prepareStats(index) + .setRequestCache(true) + .get().getTotal().getRequestCache(); // Check the hit count and miss count together so if they are not // correct we can see both values assertEquals(Arrays.asList(expectedHits, expectedMisses, 0L), diff --git a/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerTests.java b/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerTests.java index b29d3ba3b7dd4..f05684930a9c5 100644 --- a/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerTests.java +++ b/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerTests.java @@ -30,6 +30,7 @@ import org.apache.lucene.index.Term; import org.apache.lucene.search.Explanation; import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.LRUQueryCache; import org.apache.lucene.search.LeafCollector; import org.apache.lucene.search.Query; import org.apache.lucene.search.QueryCachingPolicy; @@ -47,6 +48,7 @@ import org.elasticsearch.search.internal.ContextIndexSearcher; import org.elasticsearch.search.profile.ProfileResult; import org.elasticsearch.test.ESTestCase; +import org.junit.After; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -81,7 +83,16 @@ public static void setup() throws IOException { reader = w.getReader(); w.close(); searcher = new ContextIndexSearcher(reader, IndexSearcher.getDefaultSimilarity(), - IndexSearcher.getDefaultQueryCache(), MAYBE_CACHE_POLICY); + IndexSearcher.getDefaultQueryCache(), ALWAYS_CACHE_POLICY); + } + + @After + public void checkNoCache() { + LRUQueryCache cache = (LRUQueryCache) searcher.getQueryCache(); + assertThat(cache.getHitCount(), equalTo(0L)); + assertThat(cache.getCacheCount(), equalTo(0L)); + assertThat(cache.getTotalCount(), equalTo(cache.getMissCount())); + assertThat(cache.getCacheSize(), equalTo(0L)); } @AfterClass @@ -158,10 +169,6 @@ public void testUseIndexStats() throws IOException { public void testApproximations() throws IOException { QueryProfiler profiler = new QueryProfiler(); - // disable query caching since we want to test approximations, which won't - // be exposed on a cached entry - ContextIndexSearcher searcher = new ContextIndexSearcher(reader, IndexSearcher.getDefaultSimilarity(), - null, MAYBE_CACHE_POLICY); searcher.setProfiler(profiler); Query query = new RandomApproximationQuery(new TermQuery(new Term("foo", "bar")), random()); searcher.count(query); @@ -184,7 +191,6 @@ public void testApproximations() throws IOException { long rewriteTime = profiler.getRewriteTime(); assertThat(rewriteTime, greaterThan(0L)); - } public void testCollector() throws IOException { @@ -288,17 +294,4 @@ public boolean shouldCache(Query query) throws IOException { } }; - - private static final QueryCachingPolicy NEVER_CACHE_POLICY = new QueryCachingPolicy() { - - @Override - public void onUse(Query query) {} - - @Override - public boolean shouldCache(Query query) throws IOException { - return false; - } - - }; - } From e9a0193c7960ba32e6d77b990f072a1ddabd7160 Mon Sep 17 00:00:00 2001 From: jimczi Date: Thu, 17 Oct 2019 17:24:39 +0200 Subject: [PATCH 2/4] check style --- .../java/org/elasticsearch/indices/IndicesRequestCacheIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java index a3a8436f822ba..0c5eb73385b32 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java @@ -29,7 +29,6 @@ import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.index.cache.request.RequestCacheStats; import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.index.query.TermQueryBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket; From e88b7fd8d88c78f134772815180a941aaf7fbe82 Mon Sep 17 00:00:00 2001 From: jimczi Date: Tue, 29 Oct 2019 20:10:30 +0100 Subject: [PATCH 3/4] call createWeight on the query rather the searcher in order to avoid query caching --- .../search/internal/ContextIndexSearcher.java | 19 +------------------ .../indices/IndicesRequestCacheIT.java | 2 +- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java index 33c045605c938..4bebdb0123672 100644 --- a/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java @@ -67,19 +67,6 @@ public class ContextIndexSearcher extends IndexSearcher { */ private static int CHECK_CANCELLED_SCORER_INTERVAL = 1 << 11; - /** - * A policy to bypass the cache entirely - */ - private static final QueryCachingPolicy NEVER_CACHE_POLICY = new QueryCachingPolicy() { - @Override - public void onUse(Query query) {} - - @Override - public boolean shouldCache(Query query) { - return false; - } - }; - private AggregatedDfs aggregatedDfs; private QueryProfiler profiler; private Runnable checkCancelled; @@ -92,10 +79,6 @@ public ContextIndexSearcher(IndexReader reader, Similarity similarity, QueryCach } public void setProfiler(QueryProfiler profiler) { - if (profiler != null) { - // disable query caching on profiled query - setQueryCachingPolicy(NEVER_CACHE_POLICY); - } this.profiler = profiler; } @@ -137,7 +120,7 @@ public Weight createWeight(Query query, ScoreMode scoreMode, float boost) throws timer.start(); final Weight weight; try { - weight = super.createWeight(query, scoreMode, boost); + weight = query.createWeight(this, scoreMode, boost); } finally { timer.stop(); profiler.pollLastElement(); diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java index 2d1ba65265c27..85e2cf9f2c891 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java @@ -427,7 +427,7 @@ public void testProfileDisableCache() throws Exception { ) .get() ); - indexRandom(true, client.prepareIndex("index", "_doc").setSource("k", "hello")); + indexRandom(true, client.prepareIndex("index").setSource("k", "hello")); ensureSearchable("index"); int expectedHits = 0; From eb8aabddfeea3fb3f19757d6346ddde0d77f1874 Mon Sep 17 00:00:00 2001 From: jimczi Date: Tue, 29 Oct 2019 20:13:02 +0100 Subject: [PATCH 4/4] remove an awaitsfix that is not needed anymore --- .../org/elasticsearch/search/profile/query/QueryProfilerIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java b/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java index 5ff2f3865d9e9..d272e0603240a 100644 --- a/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java +++ b/server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java @@ -54,7 +54,6 @@ public class QueryProfilerIT extends ESIntegTestCase { * This test simply checks to make sure nothing crashes. Test indexes 100-150 documents, * constructs 20-100 random queries and tries to profile them */ - @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/LUCENE-8658") public void testProfileQuery() throws Exception { createIndex("test"); ensureGreen();