Skip to content

Commit 101c419

Browse files
tlrxtlrx
authored
Use dedicated cache keys instead of relying on an absolute path (#51669)
Today cache files are identified in cache using a string representing an absolute path to a file on disk. This path is a sub directory of the current shard data path and as such already contains identification bits like the current index id and the shard id. It also contains the snapshot id that is passed at CacheDirectory creation time. While this has been done for quick prototyping and already been improved in #51520, it feels wrong to rely on a path converted to a string as cache keys. Instead we should have a distinct CacheKey object to identify CacheFile in cache. Relates #50693
1 parent b88a798 commit 101c419

File tree

7 files changed

+233
-29
lines changed

7 files changed

+233
-29
lines changed

‎x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotRepository.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ private Directory makeDirectory(IndexSettings indexSettings, ShardPath shardPath
8686
Directory directory = new SearchableSnapshotDirectory(snapshot, blobContainer);
8787
if (SNAPSHOT_CACHE_ENABLED_SETTING.get(indexSettings.getSettings())) {
8888
final Path cacheDir = shardPath.getDataPath().resolve("snapshots").resolve(snapshotId.getUUID());
89-
directory = new CacheDirectory(directory, cacheService, cacheDir);
89+
directory = new CacheDirectory(directory, cacheService, cacheDir, snapshotId, indexId, shardPath.getShardId());
9090
}
9191
directory = new InMemoryNoOpCommitDirectory(directory);
9292

‎x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/CacheDirectory.java‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
import org.elasticsearch.common.SuppressForbidden;
1616
import org.elasticsearch.common.io.Channels;
1717
import org.elasticsearch.common.util.concurrent.ReleasableLock;
18+
import org.elasticsearch.index.shard.ShardId;
19+
import org.elasticsearch.repositories.IndexId;
20+
import org.elasticsearch.snapshots.SnapshotId;
1821

1922
import java.io.EOFException;
2023
import java.io.IOException;
@@ -34,19 +37,30 @@ public class CacheDirectory extends FilterDirectory {
3437
private static final int COPY_BUFFER_SIZE = 8192;
3538

3639
private final CacheService cacheService;
40+
private final SnapshotId snapshotId;
41+
private final IndexId indexId;
42+
private final ShardId shardId;
3743
private final Path cacheDir;
3844

39-
public CacheDirectory(Directory in, CacheService cacheService, Path cacheDir) throws IOException {
45+
public CacheDirectory(Directory in, CacheService cacheService, Path cacheDir, SnapshotId snapshotId, IndexId indexId, ShardId shardId)
46+
throws IOException {
4047
super(in);
4148
this.cacheService = Objects.requireNonNull(cacheService);
4249
this.cacheDir = Files.createDirectories(cacheDir);
50+
this.snapshotId = Objects.requireNonNull(snapshotId);
51+
this.indexId = Objects.requireNonNull(indexId);
52+
this.shardId = Objects.requireNonNull(shardId);
53+
}
54+
55+
private CacheKey createCacheKey(String fileName) {
56+
return new CacheKey(snapshotId, indexId, shardId, fileName);
4357
}
4458

4559
public void close() throws IOException {
4660
super.close();
4761
// Ideally we could let the cache evict/remove cached files by itself after the
4862
// directory has been closed.
49-
cacheService.removeFromCache(key -> key.startsWith(cacheDir.toString()));
63+
cacheService.removeFromCache(cacheKey -> cacheKey.belongsTo(snapshotId, indexId, shardId));
5064
}
5165

5266
@Override
@@ -57,12 +71,12 @@ public IndexInput openInput(final String name, final IOContext context) throws I
5771

5872
private class CacheFileReference implements CacheFile.EvictionListener {
5973

60-
private final String fileName;
6174
private final long fileLength;
75+
private final CacheKey cacheKey;
6276
private final AtomicReference<CacheFile> cacheFile = new AtomicReference<>(); // null if evicted or not yet acquired
6377

6478
private CacheFileReference(String fileName, long fileLength) {
65-
this.fileName = fileName;
79+
this.cacheKey = createCacheKey(fileName);
6680
this.fileLength = fileLength;
6781
}
6882

@@ -73,7 +87,7 @@ CacheFile get() throws Exception {
7387
return currentCacheFile;
7488
}
7589

76-
final CacheFile newCacheFile = cacheService.get(fileName, fileLength, cacheDir);
90+
final CacheFile newCacheFile = cacheService.get(cacheKey, fileLength, cacheDir);
7791
synchronized (this) {
7892
currentCacheFile = cacheFile.get();
7993
if (currentCacheFile != null) {
@@ -89,7 +103,7 @@ CacheFile get() throws Exception {
89103
}
90104

91105
String getFileName() {
92-
return fileName;
106+
return cacheKey.getFileName();
93107
}
94108

95109
@Override
@@ -113,7 +127,7 @@ void releaseOnClose() {
113127
@Override
114128
public String toString() {
115129
return "CacheFileReference{" +
116-
"fileName='" + fileName + '\'' +
130+
"cacheKey='" + cacheKey + '\'' +
117131
", fileLength=" + fileLength +
118132
", cacheDir=" + cacheDir +
119133
", acquired=" + (cacheFile.get() != null) +

‎x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/CacheFile.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ protected void closeInternal() {
5050

5151
private final SparseFileTracker tracker;
5252
private final int rangeSize;
53-
private final String name;
53+
private final String description;
5454
private final Path file;
5555

5656
private volatile Set<EvictionListener> listeners;
@@ -59,9 +59,9 @@ protected void closeInternal() {
5959
@Nullable // if evicted, or there are no listeners
6060
private volatile FileChannel channel;
6161

62-
CacheFile(String name, long length, Path file, int rangeSize) {
62+
CacheFile(String description, long length, Path file, int rangeSize) {
6363
this.tracker = new SparseFileTracker(file.toString(), length);
64-
this.name = Objects.requireNonNull(name);
64+
this.description = Objects.requireNonNull(description);
6565
this.file = Objects.requireNonNull(file);
6666
this.listeners = new HashSet<>();
6767
this.rangeSize = rangeSize;
@@ -219,7 +219,7 @@ private boolean invariant() {
219219
@Override
220220
public String toString() {
221221
return "CacheFile{" +
222-
"name='" + name + '\'' +
222+
"desc='" + description + '\'' +
223223
", file=" + file +
224224
", length=" + tracker.getLength() +
225225
", channel=" + (channel != null ? "yes" : "no") +

‎x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/CacheKey.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
package org.elasticsearch.xpack.searchablesnapshots.cache;
7+
8+
import org.elasticsearch.index.shard.ShardId;
9+
import org.elasticsearch.repositories.IndexId;
10+
import org.elasticsearch.snapshots.SnapshotId;
11+
12+
import java.util.Objects;
13+
14+
public class CacheKey {
15+
16+
private final SnapshotId snapshotId;
17+
private final IndexId indexId;
18+
private final ShardId shardId;
19+
private final String fileName;
20+
21+
CacheKey(SnapshotId snapshotId, IndexId indexId, ShardId shardId, String fileName) {
22+
this.snapshotId = Objects.requireNonNull(snapshotId);
23+
this.indexId = Objects.requireNonNull(indexId);
24+
this.shardId = Objects.requireNonNull(shardId);
25+
this.fileName = Objects.requireNonNull(fileName);
26+
}
27+
28+
SnapshotId getSnapshotId() {
29+
return snapshotId;
30+
}
31+
32+
IndexId getIndexId() {
33+
return indexId;
34+
}
35+
36+
ShardId getShardId() {
37+
return shardId;
38+
}
39+
40+
String getFileName() {
41+
return fileName;
42+
}
43+
44+
@Override
45+
public boolean equals(Object o) {
46+
if (this == o) {
47+
return true;
48+
}
49+
if (o == null || getClass() != o.getClass()) {
50+
return false;
51+
}
52+
final CacheKey cacheKey = (CacheKey) o;
53+
return Objects.equals(snapshotId, cacheKey.snapshotId)
54+
&& Objects.equals(indexId, cacheKey.indexId)
55+
&& Objects.equals(shardId, cacheKey.shardId)
56+
&& Objects.equals(fileName, cacheKey.fileName);
57+
}
58+
59+
@Override
60+
public int hashCode() {
61+
return Objects.hash(snapshotId, indexId, shardId, fileName);
62+
}
63+
64+
@Override
65+
public String toString() {
66+
return "[" +
67+
"snapshotId=" + snapshotId +
68+
", indexId=" + indexId +
69+
", shardId=" + shardId +
70+
", fileName='" + fileName + '\'' +
71+
']';
72+
}
73+
74+
boolean belongsTo(SnapshotId snapshotId, IndexId indexId, ShardId shardId) {
75+
return Objects.equals(this.snapshotId, snapshotId)
76+
&& Objects.equals(this.indexId, indexId)
77+
&& Objects.equals(this.shardId, shardId);
78+
}
79+
}

‎x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/CacheService.java‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public class CacheService extends AbstractLifecycleComponent {
4040
new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES), // max
4141
Setting.Property.NodeScope);
4242

43-
private final Cache<String, CacheFile> cache;
43+
private final Cache<CacheKey, CacheFile> cache;
4444
private final ByteSizeValue cacheSize;
4545
private final ByteSizeValue rangeSize;
4646

@@ -52,7 +52,7 @@ public CacheService(final Settings settings) {
5252
CacheService(final ByteSizeValue cacheSize, final ByteSizeValue rangeSize) {
5353
this.cacheSize = Objects.requireNonNull(cacheSize);
5454
this.rangeSize = Objects.requireNonNull(rangeSize);
55-
this.cache = CacheBuilder.<String, CacheFile>builder()
55+
this.cache = CacheBuilder.<CacheKey, CacheFile>builder()
5656
.setMaximumWeight(cacheSize.getBytes())
5757
.weigher((key, entry) -> entry.getLength())
5858
// NORELEASE This does not immediately free space on disk, as cache file are only deleted when all index inputs
@@ -96,39 +96,31 @@ int getRangeSize() {
9696
return Math.toIntExact(rangeSize.getBytes());
9797
}
9898

99-
public CacheFile get(final String fileName, final long length, final Path cacheDir) throws Exception {
99+
public CacheFile get(final CacheKey cacheKey, final long fileLength, final Path cacheDir) throws Exception {
100100
ensureLifecycleStarted();
101-
return cache.computeIfAbsent(toCacheKey(cacheDir, fileName), key -> {
101+
return cache.computeIfAbsent(cacheKey, key -> {
102102
ensureLifecycleStarted();
103103
// generate a random UUID for the name of the cache file on disk
104104
final String uuid = UUIDs.randomBase64UUID();
105105
// resolve the cache file on disk w/ the expected cached file
106106
final Path path = cacheDir.resolve(uuid);
107107
assert Files.notExists(path) : "cache file already exists " + path;
108108

109-
return new CacheFile(fileName, length, path, getRangeSize());
109+
return new CacheFile(key.toString(), fileLength, path, getRangeSize());
110110
});
111111
}
112112

113113
/**
114-
* Remove from cache all entries that match the given predicate.
114+
* Invalidate cache entries with keys matching the given predicate
115115
*
116116
* @param predicate the predicate to evaluate
117117
*/
118-
void removeFromCache(final Predicate<String> predicate) {
119-
for (String cacheKey : cache.keys()) {
118+
void removeFromCache(final Predicate<CacheKey> predicate) {
119+
for (CacheKey cacheKey : cache.keys()) {
120120
if (predicate.test(cacheKey)) {
121121
cache.invalidate(cacheKey);
122122
}
123123
}
124124
cache.refresh();
125125
}
126-
127-
/**
128-
* Computes the cache key associated to the given Lucene cached file
129-
*/
130-
private static String toCacheKey(final Path cacheDir, String fileName) {
131-
// TODO Fix this. Cache Key should be computed from snapshot id/index id/shard
132-
return cacheDir.resolve(fileName).toAbsolutePath().toString();
133-
}
134126
}

‎x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/CacheBufferedIndexInputTests.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,14 @@
1313
import org.elasticsearch.common.lucene.store.ESIndexInputTestCase;
1414
import org.elasticsearch.common.unit.ByteSizeUnit;
1515
import org.elasticsearch.common.unit.ByteSizeValue;
16+
import org.elasticsearch.index.shard.ShardId;
17+
import org.elasticsearch.repositories.IndexId;
18+
import org.elasticsearch.snapshots.SnapshotId;
1619

1720
import java.io.FileNotFoundException;
1821
import java.io.IOException;
1922
import java.nio.charset.StandardCharsets;
23+
import java.nio.file.Path;
2024
import java.util.Objects;
2125
import java.util.concurrent.atomic.LongAdder;
2226

@@ -28,6 +32,10 @@ public void testRandomReads() throws IOException {
2832
try (CacheService cacheService = createCacheService()) {
2933
cacheService.start();
3034

35+
SnapshotId snapshotId = new SnapshotId("_name", "_uuid");
36+
IndexId indexId = new IndexId("_name", "_uuid");
37+
ShardId shardId = new ShardId("_name", "_uuid", 0);
38+
3139
for (int i = 0; i < 5; i++) {
3240
final String fileName = randomAlphaOfLength(10);
3341
final byte[] input = randomUnicodeOfLength(randomIntBetween(1, 100_000)).getBytes(StandardCharsets.UTF_8);
@@ -37,7 +45,8 @@ public void testRandomReads() throws IOException {
3745
directory = new CountingDirectory(directory, cacheService.getRangeSize());
3846
}
3947

40-
try (CacheDirectory cacheDirectory = new CacheDirectory(directory, cacheService, createTempDir())) {
48+
final Path cacheDir = createTempDir();
49+
try (CacheDirectory cacheDirectory = new CacheDirectory(directory, cacheService, cacheDir, snapshotId, indexId, shardId)) {
4150
try (IndexInput indexInput = cacheDirectory.openInput(fileName, newIOContext(random()))) {
4251
assertEquals(input.length, indexInput.length());
4352
assertEquals(0, indexInput.getFilePointer());

0 commit comments

Comments
 (0)