Skip to content

Commit 00272ea

Browse files
javannajavanna
authored
Remove cache key renderer argument from IndicesRequestCache (elastic#62534)
In the context of of a recurring test failure tracked by elastic#32827, we added trace logging and an extra cache key renderer argument to IndicesRequestCache#getOrCompute (see elastic#39475 and elastic#34180). We addressed the issue with elastic#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 elastic#55837
1 parent 8aeab0b commit 00272ea

File tree

4 files changed

+23
-36
lines changed

4 files changed

+23
-36
lines changed

‎server/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
import com.carrotsearch.hppc.ObjectHashSet;
2323
import com.carrotsearch.hppc.ObjectSet;
24-
2524
import org.apache.logging.log4j.LogManager;
2625
import org.apache.logging.log4j.Logger;
2726
import org.apache.lucene.index.DirectoryReader;
@@ -51,7 +50,6 @@
5150
import java.util.Objects;
5251
import java.util.Set;
5352
import java.util.concurrent.ConcurrentMap;
54-
import java.util.function.Supplier;
5553

5654
/**
5755
* The indices request cache allows to cache a shard level request stage responses, helping with improving
@@ -114,20 +112,14 @@ public void onRemoval(RemovalNotification<Key, BytesReference> notification) {
114112
notification.getKey().entity.onRemoval(notification);
115113
}
116114

117-
// NORELEASE The cacheKeyRenderer has been added in order to debug
118-
// https://github.com/elastic/elasticsearch/issues/32827, it should be
119-
// removed when this issue is solved
120115
BytesReference getOrCompute(CacheEntity cacheEntity, CheckedSupplier<BytesReference, IOException> loader,
121-
DirectoryReader reader, BytesReference cacheKey, Supplier<String> cacheKeyRenderer) throws Exception {
116+
DirectoryReader reader, BytesReference cacheKey) throws Exception {
122117
assert reader.getReaderCacheHelper() != null;
123118
final Key key = new Key(cacheEntity, reader.getReaderCacheHelper().getKey(), cacheKey);
124119
Loader cacheLoader = new Loader(cacheEntity, loader);
125120
BytesReference value = cache.computeIfAbsent(key, cacheLoader);
126121
if (cacheLoader.isLoaded()) {
127122
key.entity.onMiss();
128-
if (logger.isTraceEnabled()) {
129-
logger.trace("Cache miss for reader version [{}] and request:\n {}", reader.getVersion(), cacheKeyRenderer.get());
130-
}
131123
// see if its the first time we see this reader, and make sure to register a cleanup key
132124
CleanupKey cleanupKey = new CleanupKey(cacheEntity, reader.getReaderCacheHelper().getKey());
133125
if (!registeredClosedListeners.containsKey(cleanupKey)) {
@@ -138,9 +130,6 @@ BytesReference getOrCompute(CacheEntity cacheEntity, CheckedSupplier<BytesRefere
138130
}
139131
} else {
140132
key.entity.onHit();
141-
if (logger.isTraceEnabled()) {
142-
logger.trace("Cache hit for reader version [{}] and request:\n {}", reader.getVersion(), cacheKeyRenderer.get());
143-
}
144133
}
145134
return value;
146135
}

‎server/src/main/java/org/elasticsearch/indices/IndicesService.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,6 @@
161161
import java.util.function.Function;
162162
import java.util.function.LongSupplier;
163163
import java.util.function.Predicate;
164-
import java.util.function.Supplier;
165164
import java.util.stream.Collectors;
166165

167166
import static java.util.Collections.emptyList;
@@ -1388,7 +1387,6 @@ public void loadIntoContext(ShardSearchRequest request, SearchContext context, Q
13881387

13891388
boolean[] loadedFromCache = new boolean[] { true };
13901389
BytesReference bytesReference = cacheShardLevelResult(context.indexShard(), directoryReader, request.cacheKey(),
1391-
() -> "Shard: " + request.shardId() + "\nSource:\n" + request.source(),
13921390
out -> {
13931391
queryPhase.execute(context);
13941392
context.queryResult().writeToNoId(out);
@@ -1430,7 +1428,7 @@ public ByteSizeValue getTotalIndexingBufferBytes() {
14301428
* @return the contents of the cache or the result of calling the loader
14311429
*/
14321430
private BytesReference cacheShardLevelResult(IndexShard shard, DirectoryReader reader, BytesReference cacheKey,
1433-
Supplier<String> cacheKeyRenderer, CheckedConsumer<StreamOutput, IOException> loader) throws Exception {
1431+
CheckedConsumer<StreamOutput, IOException> loader) throws Exception {
14341432
IndexShardCacheEntity cacheEntity = new IndexShardCacheEntity(shard);
14351433
CheckedSupplier<BytesReference, IOException> supplier = () -> {
14361434
/* BytesStreamOutput allows to pass the expected size but by default uses
@@ -1448,7 +1446,7 @@ private BytesReference cacheShardLevelResult(IndexShard shard, DirectoryReader r
14481446
return out.bytes();
14491447
}
14501448
};
1451-
return indicesRequestCache.getOrCompute(cacheEntity, supplier, reader, cacheKey, cacheKeyRenderer);
1449+
return indicesRequestCache.getOrCompute(cacheEntity, supplier, reader, cacheKey);
14521450
}
14531451

14541452
static final class IndexShardCacheEntity extends AbstractIndexShardCacheEntity {

‎server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public void testBasicOperationsCache() throws Exception {
6969
// initial cache
7070
TestEntity entity = new TestEntity(requestCacheStats, indexShard);
7171
Loader loader = new Loader(reader, 0);
72-
BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
72+
BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes);
7373
assertEquals("foo", value.streamInput().readString());
7474
assertEquals(0, requestCacheStats.stats().getHitCount());
7575
assertEquals(1, requestCacheStats.stats().getMissCount());
@@ -80,7 +80,7 @@ public void testBasicOperationsCache() throws Exception {
8080
// cache hit
8181
entity = new TestEntity(requestCacheStats, indexShard);
8282
loader = new Loader(reader, 0);
83-
value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
83+
value = cache.getOrCompute(entity, loader, reader, termBytes);
8484
assertEquals("foo", value.streamInput().readString());
8585
assertEquals(1, requestCacheStats.stats().getHitCount());
8686
assertEquals(1, requestCacheStats.stats().getMissCount());
@@ -131,7 +131,7 @@ public void testCacheDifferentReaders() throws Exception {
131131
// initial cache
132132
TestEntity entity = new TestEntity(requestCacheStats, indexShard);
133133
Loader loader = new Loader(reader, 0);
134-
BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
134+
BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes);
135135
assertEquals("foo", value.streamInput().readString());
136136
assertEquals(0, requestCacheStats.stats().getHitCount());
137137
assertEquals(1, requestCacheStats.stats().getMissCount());
@@ -145,7 +145,7 @@ public void testCacheDifferentReaders() throws Exception {
145145
// cache the second
146146
TestEntity secondEntity = new TestEntity(requestCacheStats, indexShard);
147147
loader = new Loader(secondReader, 0);
148-
value = cache.getOrCompute(entity, loader, secondReader, termBytes, () -> termQuery.toString());
148+
value = cache.getOrCompute(entity, loader, secondReader, termBytes);
149149
assertEquals("bar", value.streamInput().readString());
150150
assertEquals(0, requestCacheStats.stats().getHitCount());
151151
assertEquals(2, requestCacheStats.stats().getMissCount());
@@ -157,7 +157,7 @@ public void testCacheDifferentReaders() throws Exception {
157157

158158
secondEntity = new TestEntity(requestCacheStats, indexShard);
159159
loader = new Loader(secondReader, 0);
160-
value = cache.getOrCompute(secondEntity, loader, secondReader, termBytes, () -> termQuery.toString());
160+
value = cache.getOrCompute(secondEntity, loader, secondReader, termBytes);
161161
assertEquals("bar", value.streamInput().readString());
162162
assertEquals(1, requestCacheStats.stats().getHitCount());
163163
assertEquals(2, requestCacheStats.stats().getMissCount());
@@ -167,7 +167,7 @@ public void testCacheDifferentReaders() throws Exception {
167167

168168
entity = new TestEntity(requestCacheStats, indexShard);
169169
loader = new Loader(reader, 0);
170-
value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
170+
value = cache.getOrCompute(entity, loader, reader, termBytes);
171171
assertEquals("foo", value.streamInput().readString());
172172
assertEquals(2, requestCacheStats.stats().getHitCount());
173173
assertEquals(2, requestCacheStats.stats().getMissCount());
@@ -227,9 +227,9 @@ public void testEviction() throws Exception {
227227
TestEntity secondEntity = new TestEntity(requestCacheStats, indexShard);
228228
Loader secondLoader = new Loader(secondReader, 0);
229229

230-
BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
230+
BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes);
231231
assertEquals("foo", value1.streamInput().readString());
232-
BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes, () -> termQuery.toString());
232+
BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes);
233233
assertEquals("bar", value2.streamInput().readString());
234234
size = requestCacheStats.stats().getMemorySize();
235235
IOUtils.close(reader, secondReader, writer, dir, cache);
@@ -262,12 +262,12 @@ public void testEviction() throws Exception {
262262
TestEntity thirddEntity = new TestEntity(requestCacheStats, indexShard);
263263
Loader thirdLoader = new Loader(thirdReader, 0);
264264

265-
BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
265+
BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes);
266266
assertEquals("foo", value1.streamInput().readString());
267-
BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes, () -> termQuery.toString());
267+
BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes);
268268
assertEquals("bar", value2.streamInput().readString());
269269
logger.info("Memory size: {}", requestCacheStats.stats().getMemorySize());
270-
BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes, () -> termQuery.toString());
270+
BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes);
271271
assertEquals("baz", value3.streamInput().readString());
272272
assertEquals(2, cache.count());
273273
assertEquals(1, requestCacheStats.stats().getEvictions());
@@ -303,12 +303,12 @@ public void testClearAllEntityIdentity() throws Exception {
303303
TestEntity thirddEntity = new TestEntity(requestCacheStats, differentIdentity);
304304
Loader thirdLoader = new Loader(thirdReader, 0);
305305

306-
BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
306+
BytesReference value1 = cache.getOrCompute(entity, loader, reader, termBytes);
307307
assertEquals("foo", value1.streamInput().readString());
308-
BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes, () -> termQuery.toString());
308+
BytesReference value2 = cache.getOrCompute(secondEntity, secondLoader, secondReader, termBytes);
309309
assertEquals("bar", value2.streamInput().readString());
310310
logger.info("Memory size: {}", requestCacheStats.stats().getMemorySize());
311-
BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes, () -> termQuery.toString());
311+
BytesReference value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes);
312312
assertEquals("baz", value3.streamInput().readString());
313313
assertEquals(3, cache.count());
314314
final long hitCount = requestCacheStats.stats().getHitCount();
@@ -317,7 +317,7 @@ public void testClearAllEntityIdentity() throws Exception {
317317
cache.cleanCache();
318318
assertEquals(1, cache.count());
319319
// third has not been validated since it's a different identity
320-
value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes, () -> termQuery.toString());
320+
value3 = cache.getOrCompute(thirddEntity, thirdLoader, thirdReader, termBytes);
321321
assertEquals(hitCount + 1, requestCacheStats.stats().getHitCount());
322322
assertEquals("baz", value3.streamInput().readString());
323323

@@ -376,7 +376,7 @@ public void testInvalidate() throws Exception {
376376
// initial cache
377377
TestEntity entity = new TestEntity(requestCacheStats, indexShard);
378378
Loader loader = new Loader(reader, 0);
379-
BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
379+
BytesReference value = cache.getOrCompute(entity, loader, reader, termBytes);
380380
assertEquals("foo", value.streamInput().readString());
381381
assertEquals(0, requestCacheStats.stats().getHitCount());
382382
assertEquals(1, requestCacheStats.stats().getMissCount());
@@ -387,7 +387,7 @@ public void testInvalidate() throws Exception {
387387
// cache hit
388388
entity = new TestEntity(requestCacheStats, indexShard);
389389
loader = new Loader(reader, 0);
390-
value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
390+
value = cache.getOrCompute(entity, loader, reader, termBytes);
391391
assertEquals("foo", value.streamInput().readString());
392392
assertEquals(1, requestCacheStats.stats().getHitCount());
393393
assertEquals(1, requestCacheStats.stats().getMissCount());
@@ -401,7 +401,7 @@ public void testInvalidate() throws Exception {
401401
entity = new TestEntity(requestCacheStats, indexShard);
402402
loader = new Loader(reader, 0);
403403
cache.invalidate(entity, reader, termBytes);
404-
value = cache.getOrCompute(entity, loader, reader, termBytes, () -> termQuery.toString());
404+
value = cache.getOrCompute(entity, loader, reader, termBytes);
405405
assertEquals("foo", value.streamInput().readString());
406406
assertEquals(1, requestCacheStats.stats().getHitCount());
407407
assertEquals(2, requestCacheStats.stats().getMissCount());

‎server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@
4848

4949
import java.nio.file.Path;
5050
import java.util.Arrays;
51-
import java.util.concurrent.TimeUnit;
5251
import java.util.Collections;
52+
import java.util.concurrent.TimeUnit;
5353

5454
import static org.elasticsearch.cluster.coordination.ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING;
5555
import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS;
@@ -287,7 +287,7 @@ public void onMiss() {}
287287
@Override
288288
public void onRemoval(RemovalNotification<Key, BytesReference> notification) {}
289289
};
290-
cache.getOrCompute(cacheEntity, () -> new BytesArray("bar"), searcher.getDirectoryReader(), new BytesArray("foo"), () -> "foo");
290+
cache.getOrCompute(cacheEntity, () -> new BytesArray("bar"), searcher.getDirectoryReader(), new BytesArray("foo"));
291291
assertEquals(1L, cache.count());
292292

293293
searcher.close();

0 commit comments

Comments
 (0)