Skip to content

Commit 8935fff

Browse files
javannajavanna
authored
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
1 parent 2b0418d commit 8935fff

File tree

5 files changed

+23
-42
lines changed

5 files changed

+23
-42
lines changed

‎server/src/internalClusterTest/java/org/elasticsearch/indices/IndicesRequestCacheIT.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket;
3636
import org.elasticsearch.test.ESIntegTestCase;
3737
import org.elasticsearch.test.hamcrest.ElasticsearchAssertions;
38-
import org.elasticsearch.test.junit.annotations.TestIssueLogging;
3938

4039
import java.time.ZoneId;
4140
import java.time.ZoneOffset;
@@ -98,9 +97,6 @@ public void testCacheAggs() throws Exception {
9897
}
9998
}
10099

101-
@TestIssueLogging(
102-
value = "org.elasticsearch.indices.IndicesRequestCache:TRACE",
103-
issueUrl = "https://github.com/elastic/elasticsearch/issues/32827")
104100
public void testQueryRewrite() throws Exception {
105101
Client client = client();
106102
assertAcked(client.admin().indices().prepareCreate("index").setMapping("s", "type=date")

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

Lines changed: 1 addition & 14 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,21 +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 [{}], max_doc[{}] and request:\n {}",
130-
reader.getVersion(), reader.maxDoc(), cacheKeyRenderer.get());
131-
}
132123
// see if its the first time we see this reader, and make sure to register a cleanup key
133124
CleanupKey cleanupKey = new CleanupKey(cacheEntity, reader.getReaderCacheHelper().getKey());
134125
if (!registeredClosedListeners.containsKey(cleanupKey)) {
@@ -139,10 +130,6 @@ BytesReference getOrCompute(CacheEntity cacheEntity, CheckedSupplier<BytesRefere
139130
}
140131
} else {
141132
key.entity.onHit();
142-
if (logger.isTraceEnabled()) {
143-
logger.trace("Cache hit for reader version [{}], max_doc[{}] and request:\n {}",
144-
reader.getVersion(), reader.maxDoc(), cacheKeyRenderer.get());
145-
}
146133
}
147134
return value;
148135
}

‎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;
@@ -1386,7 +1385,6 @@ public void loadIntoContext(ShardSearchRequest request, SearchContext context, Q
13861385

13871386
boolean[] loadedFromCache = new boolean[] { true };
13881387
BytesReference bytesReference = cacheShardLevelResult(context.indexShard(), directoryReader, request.cacheKey(),
1389-
() -> "Shard: " + request.shardId() + "\nSource:\n" + request.source(),
13901388
out -> {
13911389
queryPhase.execute(context);
13921390
context.queryResult().writeToNoId(out);
@@ -1428,7 +1426,7 @@ public ByteSizeValue getTotalIndexingBufferBytes() {
14281426
* @return the contents of the cache or the result of calling the loader
14291427
*/
14301428
private BytesReference cacheShardLevelResult(IndexShard shard, DirectoryReader reader, BytesReference cacheKey,
1431-
Supplier<String> cacheKeyRenderer, CheckedConsumer<StreamOutput, IOException> loader) throws Exception {
1429+
CheckedConsumer<StreamOutput, IOException> loader) throws Exception {
14321430
IndexShardCacheEntity cacheEntity = new IndexShardCacheEntity(shard);
14331431
CheckedSupplier<BytesReference, IOException> supplier = () -> {
14341432
/* BytesStreamOutput allows to pass the expected size but by default uses
@@ -1446,7 +1444,7 @@ private BytesReference cacheShardLevelResult(IndexShard shard, DirectoryReader r
14461444
return out.bytes();
14471445
}
14481446
};
1449-
return indicesRequestCache.getOrCompute(cacheEntity, supplier, reader, cacheKey, cacheKeyRenderer);
1447+
return indicesRequestCache.getOrCompute(cacheEntity, supplier, reader, cacheKey);
14501448
}
14511449

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