From 81bbb1becb403ba0aee1fcbf4b2d8bb2cc427e3a Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Fri, 18 Sep 2020 16:53:37 +0200 Subject: [PATCH] Remove cache key renderer argument from IndicesRequestCache (#62534) In the context of of a recurring test failure tracked by #32827, we added trace logging and an extra cache key renderer argument to IndicesRequestCache#getOrCompute (see #39475 and #34180). We addressed the issue with #54071, but the extra argument was left behind, with a NORELEASE comment saying it should be removed. With this commit, we remove the extra cache key rendered argument and the corresponding log lines which are not so useful without it. Closes #55837 --- .../indices/IndicesRequestCache.java | 13 +------ .../elasticsearch/indices/IndicesService.java | 6 ++-- .../indices/IndicesRequestCacheTests.java | 36 +++++++++---------- .../indices/IndicesServiceCloseTests.java | 4 +-- 4 files changed, 23 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java b/server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java index 78dbde7aa3c87..973595b084b7e 100644 --- a/server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java +++ b/server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java @@ -21,7 +21,6 @@ import com.carrotsearch.hppc.ObjectHashSet; import com.carrotsearch.hppc.ObjectSet; - import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.index.DirectoryReader; @@ -51,7 +50,6 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentMap; -import java.util.function.Supplier; /** * The indices request cache allows to cache a shard level request stage responses, helping with improving @@ -114,20 +112,14 @@ public void onRemoval(RemovalNotification notification) { notification.getKey().entity.onRemoval(notification); } - // NORELEASE The cacheKeyRenderer has been added in order to debug - // https://github.com/elastic/elasticsearch/issues/32827, it should be - // removed when this issue is solved BytesReference getOrCompute(CacheEntity cacheEntity, CheckedSupplier loader, - DirectoryReader reader, BytesReference cacheKey, Supplier cacheKeyRenderer) throws Exception { + DirectoryReader reader, BytesReference cacheKey) throws Exception { assert reader.getReaderCacheHelper() != null; final Key key = new Key(cacheEntity, reader.getReaderCacheHelper().getKey(), cacheKey); Loader cacheLoader = new Loader(cacheEntity, loader); BytesReference value = cache.computeIfAbsent(key, cacheLoader); if (cacheLoader.isLoaded()) { key.entity.onMiss(); - if (logger.isTraceEnabled()) { - logger.trace("Cache miss for reader version [{}] and request:\n {}", reader.getVersion(), cacheKeyRenderer.get()); - } // see if its the first time we see this reader, and make sure to register a cleanup key CleanupKey cleanupKey = new CleanupKey(cacheEntity, reader.getReaderCacheHelper().getKey()); if (!registeredClosedListeners.containsKey(cleanupKey)) { @@ -138,9 +130,6 @@ BytesReference getOrCompute(CacheEntity cacheEntity, CheckedSupplier "Shard: " + request.shardId() + "\nSource:\n" + request.source(), out -> { queryPhase.execute(context); context.queryResult().writeToNoId(out); @@ -1430,7 +1428,7 @@ public ByteSizeValue getTotalIndexingBufferBytes() { * @return the contents of the cache or the result of calling the loader */ private BytesReference cacheShardLevelResult(IndexShard shard, DirectoryReader reader, BytesReference cacheKey, - Supplier cacheKeyRenderer, CheckedConsumer loader) throws Exception { + CheckedConsumer loader) throws Exception { IndexShardCacheEntity cacheEntity = new IndexShardCacheEntity(shard); CheckedSupplier supplier = () -> { /* BytesStreamOutput allows to pass the expected size but by default uses @@ -1448,7 +1446,7 @@ private BytesReference cacheShardLevelResult(IndexShard shard, DirectoryReader r return out.bytes(); } }; - return indicesRequestCache.getOrCompute(cacheEntity, supplier, reader, cacheKey, cacheKeyRenderer); + return indicesRequestCache.getOrCompute(cacheEntity, supplier, reader, cacheKey); } static final class IndexShardCacheEntity extends AbstractIndexShardCacheEntity { diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java index 6f734f55dd372..e4eb70c3df03a 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java @@ -69,7 +69,7 @@ public void testBasicOperationsCache() throws Exception { // initial cache TestEntity entity = new TestEntity(requestCacheStats, indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(0, requestCacheStats.stats().getHitCount()); assertEquals(1, requestCacheStats.stats().getMissCount()); @@ -80,7 +80,7 @@ public void testBasicOperationsCache() throws Exception { // cache hit entity = new TestEntity(requestCacheStats, indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(1, requestCacheStats.stats().getHitCount()); assertEquals(1, requestCacheStats.stats().getMissCount()); @@ -131,7 +131,7 @@ public void testCacheDifferentReaders() throws Exception { // initial cache TestEntity entity = new TestEntity(requestCacheStats, indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(0, requestCacheStats.stats().getHitCount()); assertEquals(1, requestCacheStats.stats().getMissCount()); @@ -145,7 +145,7 @@ public void testCacheDifferentReaders() throws Exception { // cache the second TestEntity secondEntity = new TestEntity(requestCacheStats, indexShard); loader = new Loader(secondReader, 0); - value = cache.getOrCompute(entity, loader, secondReader, termBytes, () -> termQuery.toString()); + value = cache.getOrCompute(entity, loader, secondReader, termBytes); assertEquals("bar", value.streamInput().readString()); assertEquals(0, requestCacheStats.stats().getHitCount()); assertEquals(2, requestCacheStats.stats().getMissCount()); @@ -157,7 +157,7 @@ public void testCacheDifferentReaders() throws Exception { secondEntity = new TestEntity(requestCacheStats, indexShard); loader = new Loader(secondReader, 0); - value = cache.getOrCompute(secondEntity, loader, secondReader, termBytes, () -> termQuery.toString()); + value = cache.getOrCompute(secondEntity, loader, secondReader, termBytes); assertEquals("bar", value.streamInput().readString()); assertEquals(1, requestCacheStats.stats().getHitCount()); assertEquals(2, requestCacheStats.stats().getMissCount()); @@ -167,7 +167,7 @@ public void testCacheDifferentReaders() throws Exception { entity = new TestEntity(requestCacheStats, indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(2, requestCacheStats.stats().getHitCount()); assertEquals(2, requestCacheStats.stats().getMissCount()); @@ -227,9 +227,9 @@ public void testEviction() throws Exception { TestEntity secondEntity = new TestEntity(requestCacheStats, indexShard); Loader secondLoader = new Loader(secondReader, 0); - BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value1.streamInput().readString()); - BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes, () -> termQuery.toString()); + BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); assertEquals("bar", value2.streamInput().readString()); size = requestCacheStats.stats().getMemorySize(); IOUtils.close(reader, secondReader, writer, dir, cache); @@ -262,12 +262,12 @@ public void testEviction() throws Exception { TestEntity thirddEntity = new TestEntity(requestCacheStats, indexShard); Loader thirdLoader = new Loader(thirdReader, 0); - BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value1.streamInput().readString()); - BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes, () -> termQuery.toString()); + BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); assertEquals("bar", value2.streamInput().readString()); logger.info("Memory size: {}", requestCacheStats.stats().getMemorySize()); - BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes, () -> termQuery.toString()); + BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes); assertEquals("baz", value3.streamInput().readString()); assertEquals(2, cache.count()); assertEquals(1, requestCacheStats.stats().getEvictions()); @@ -303,12 +303,12 @@ public void testClearAllEntityIdentity() throws Exception { TestEntity thirddEntity = new TestEntity(requestCacheStats, differentIdentity); Loader thirdLoader = new Loader(thirdReader, 0); - BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value1.streamInput().readString()); - BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes, () -> termQuery.toString()); + BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes); assertEquals("bar", value2.streamInput().readString()); logger.info("Memory size: {}", requestCacheStats.stats().getMemorySize()); - BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes, () -> termQuery.toString()); + BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes); assertEquals("baz", value3.streamInput().readString()); assertEquals(3, cache.count()); final long hitCount = requestCacheStats.stats().getHitCount(); @@ -317,7 +317,7 @@ public void testClearAllEntityIdentity() throws Exception { cache.cleanCache(); assertEquals(1, cache.count()); // third has not been validated since it's a different identity - value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes, () -> termQuery.toString()); + value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes); assertEquals(hitCount + 1, requestCacheStats.stats().getHitCount()); assertEquals("baz", value3.streamInput().readString()); @@ -376,7 +376,7 @@ public void testInvalidate() throws Exception { // initial cache TestEntity entity = new TestEntity(requestCacheStats, indexShard); Loader loader = new Loader(reader, 0); - BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(0, requestCacheStats.stats().getHitCount()); assertEquals(1, requestCacheStats.stats().getMissCount()); @@ -387,7 +387,7 @@ public void testInvalidate() throws Exception { // cache hit entity = new TestEntity(requestCacheStats, indexShard); loader = new Loader(reader, 0); - value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(1, requestCacheStats.stats().getHitCount()); assertEquals(1, requestCacheStats.stats().getMissCount()); @@ -401,7 +401,7 @@ public void testInvalidate() throws Exception { entity = new TestEntity(requestCacheStats, indexShard); loader = new Loader(reader, 0); cache.invalidate(entity, reader, termBytes); - value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString()); + value = cache.getOrCompute(entity, loader, reader, termBytes); assertEquals("foo", value.streamInput().readString()); assertEquals(1, requestCacheStats.stats().getHitCount()); assertEquals(2, requestCacheStats.stats().getMissCount()); diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java index 7fbd0816c2d64..f5bfd740e11de 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java @@ -48,8 +48,8 @@ import java.nio.file.Path; import java.util.Arrays; -import java.util.concurrent.TimeUnit; import java.util.Collections; +import java.util.concurrent.TimeUnit; import static org.elasticsearch.cluster.coordination.ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING; import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; @@ -287,7 +287,7 @@ public void onMiss() {} @Override public void onRemoval(RemovalNotification notification) {} }; - cache.getOrCompute(cacheEntity, () -> new BytesArray("bar"), searcher.getDirectoryReader(), new BytesArray("foo"), () -> "foo"); + cache.getOrCompute(cacheEntity, () -> new BytesArray("bar"), searcher.getDirectoryReader(), new BytesArray("foo")); assertEquals(1L, cache.count()); searcher.close();