Skip to content

Commit c4249a8

Browse files
PeteGillinElasticPeteGillinElastic
authored
ES-11372: Add project ID to key for geoip cache (#129572)
In this change, the lazy loader passes in the default project ID. That will be fixed in a later PR. ES-11372 #comment Project ID added to cache key in #129572
1 parent b25d7b9 commit c4249a8

File tree

3 files changed

+81
-32
lines changed

3 files changed

+81
-32
lines changed

‎modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,11 @@
1616
import org.apache.logging.log4j.Logger;
1717
import org.apache.lucene.util.SetOnce;
1818
import org.elasticsearch.ExceptionsHelper;
19+
import org.elasticsearch.cluster.metadata.ProjectId;
1920
import org.elasticsearch.common.CheckedBiFunction;
2021
import org.elasticsearch.common.CheckedSupplier;
2122
import org.elasticsearch.core.Booleans;
23+
import org.elasticsearch.core.FixForMultiProject;
2224
import org.elasticsearch.core.IOUtils;
2325
import org.elasticsearch.core.Nullable;
2426
import org.elasticsearch.core.SuppressForbidden;
@@ -105,8 +107,9 @@ int current() {
105107

106108
@Override
107109
@Nullable
110+
@FixForMultiProject // do not use ProjectId.DEFAULT
108111
public <RESPONSE> RESPONSE getResponse(String ipAddress, CheckedBiFunction<Reader, String, RESPONSE, Exception> responseProvider) {
109-
return cache.putIfAbsent(ipAddress, cachedDatabasePathToString, ip -> {
112+
return cache.putIfAbsent(ProjectId.DEFAULT, ipAddress, cachedDatabasePathToString, ip -> {
110113
try {
111114
return responseProvider.apply(get(), ipAddress);
112115
} catch (Exception e) {
@@ -143,9 +146,10 @@ public void shutdown() throws IOException {
143146
}
144147

145148
// Visible for Testing
149+
@FixForMultiProject // do not use ProjectId.DEFAULT
146150
protected void doShutdown() throws IOException {
147151
IOUtils.close(databaseReader.get());
148-
int numEntriesEvicted = cache.purgeCacheEntriesForDatabase(databasePath);
152+
int numEntriesEvicted = cache.purgeCacheEntriesForDatabase(ProjectId.DEFAULT, databasePath);
149153
logger.info("evicted [{}] entries from cache after reloading database [{}]", numEntriesEvicted, databasePath);
150154
if (deleteDatabaseFileOnShutdown) {
151155
logger.info("deleting [{}]", databasePath);

‎modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
import com.maxmind.db.NodeCache;
1212

13+
import org.elasticsearch.cluster.metadata.ProjectId;
1314
import org.elasticsearch.common.cache.Cache;
1415
import org.elasticsearch.common.cache.CacheBuilder;
1516
import org.elasticsearch.core.TimeValue;
@@ -60,9 +61,9 @@ public String toString() {
6061
}
6162

6263
@SuppressWarnings("unchecked")
63-
<RESPONSE> RESPONSE putIfAbsent(String ip, String databasePath, Function<String, RESPONSE> retrieveFunction) {
64+
<RESPONSE> RESPONSE putIfAbsent(ProjectId projectId, String ip, String databasePath, Function<String, RESPONSE> retrieveFunction) {
6465
// can't use cache.computeIfAbsent due to the elevated permissions for the jackson (run via the cache loader)
65-
CacheKey cacheKey = new CacheKey(ip, databasePath);
66+
CacheKey cacheKey = new CacheKey(projectId, ip, databasePath);
6667
long cacheStart = relativeNanoTimeProvider.getAsLong();
6768
// intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition.
6869
Object response = cache.get(cacheKey);
@@ -92,16 +93,16 @@ <RESPONSE> RESPONSE putIfAbsent(String ip, String databasePath, Function<String,
9293
}
9394

9495
// only useful for testing
95-
Object get(String ip, String databasePath) {
96-
CacheKey cacheKey = new CacheKey(ip, databasePath);
96+
Object get(ProjectId projectId, String ip, String databasePath) {
97+
CacheKey cacheKey = new CacheKey(projectId, ip, databasePath);
9798
return cache.get(cacheKey);
9899
}
99100

100-
public int purgeCacheEntriesForDatabase(Path databaseFile) {
101+
public int purgeCacheEntriesForDatabase(ProjectId projectId, Path databaseFile) {
101102
String databasePath = databaseFile.toString();
102103
int counter = 0;
103104
for (CacheKey key : cache.keys()) {
104-
if (key.databasePath.equals(databasePath)) {
105+
if (key.projectId.equals(projectId) && key.databasePath.equals(databasePath)) {
105106
cache.invalidate(key);
106107
counter++;
107108
}
@@ -135,5 +136,5 @@ public CacheStats getCacheStats() {
135136
* path is needed to be included in the cache key. For example, if we only used the IP address as the key the City and ASN the same
136137
* IP may be in both with different values and we need to cache both.
137138
*/
138-
private record CacheKey(String ip, String databasePath) {}
139+
private record CacheKey(ProjectId projectId, String ip, String databasePath) {}
139140
}

‎modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java‎

Lines changed: 67 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
import com.maxmind.geoip2.model.AbstractResponse;
1313

14+
import org.elasticsearch.cluster.metadata.ProjectId;
15+
import org.elasticsearch.core.PathUtils;
1416
import org.elasticsearch.core.TimeValue;
1517
import org.elasticsearch.ingest.geoip.stats.CacheStats;
1618
import org.elasticsearch.test.ESTestCase;
@@ -26,21 +28,22 @@ public class GeoIpCacheTests extends ESTestCase {
2628

2729
public void testCachesAndEvictsResults() {
2830
GeoIpCache cache = new GeoIpCache(1);
31+
ProjectId projectId = randomProjectIdOrDefault();
2932
AbstractResponse response1 = mock(AbstractResponse.class);
3033
AbstractResponse response2 = mock(AbstractResponse.class);
3134

3235
// add a key
33-
AbstractResponse cachedResponse = cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1);
36+
AbstractResponse cachedResponse = cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db", ip -> response1);
3437
assertSame(cachedResponse, response1);
35-
assertSame(cachedResponse, cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1));
36-
assertSame(cachedResponse, cache.get("127.0.0.1", "path/to/db"));
38+
assertSame(cachedResponse, cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db", ip -> response1));
39+
assertSame(cachedResponse, cache.get(projectId, "127.0.0.1", "path/to/db"));
3740

3841
// evict old key by adding another value
39-
cachedResponse = cache.putIfAbsent("127.0.0.2", "path/to/db", ip -> response2);
42+
cachedResponse = cache.putIfAbsent(projectId, "127.0.0.2", "path/to/db", ip -> response2);
4043
assertSame(cachedResponse, response2);
41-
assertSame(cachedResponse, cache.putIfAbsent("127.0.0.2", "path/to/db", ip -> response2));
42-
assertSame(cachedResponse, cache.get("127.0.0.2", "path/to/db"));
43-
assertNotSame(response1, cache.get("127.0.0.1", "path/to/db"));
44+
assertSame(cachedResponse, cache.putIfAbsent(projectId, "127.0.0.2", "path/to/db", ip -> response2));
45+
assertSame(cachedResponse, cache.get(projectId, "127.0.0.2", "path/to/db"));
46+
assertNotSame(response1, cache.get(projectId, "127.0.0.1", "path/to/db"));
4447
}
4548

4649
public void testCachesNoResult() {
@@ -51,31 +54,47 @@ public void testCachesNoResult() {
5154
return null;
5255
};
5356

54-
AbstractResponse response = cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull);
57+
ProjectId projectId = randomProjectIdOrDefault();
58+
AbstractResponse response = cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db", countAndReturnNull);
5559
assertNull(response);
56-
assertNull(cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull));
60+
assertNull(cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db", countAndReturnNull));
5761
assertEquals(1, count.get());
5862

5963
// the cached value is not actually *null*, it's the NO_RESULT sentinel
60-
assertSame(GeoIpCache.NO_RESULT, cache.get("127.0.0.1", "path/to/db"));
64+
assertSame(GeoIpCache.NO_RESULT, cache.get(projectId, "127.0.0.1", "path/to/db"));
6165
}
6266

63-
public void testCacheKey() {
67+
public void testCacheDoesNotCollideForDifferentDatabases() {
6468
GeoIpCache cache = new GeoIpCache(2);
6569
AbstractResponse response1 = mock(AbstractResponse.class);
6670
AbstractResponse response2 = mock(AbstractResponse.class);
6771

68-
assertSame(response1, cache.putIfAbsent("127.0.0.1", "path/to/db1", ip -> response1));
69-
assertSame(response2, cache.putIfAbsent("127.0.0.1", "path/to/db2", ip -> response2));
70-
assertSame(response1, cache.get("127.0.0.1", "path/to/db1"));
71-
assertSame(response2, cache.get("127.0.0.1", "path/to/db2"));
72+
ProjectId projectId = randomProjectIdOrDefault();
73+
assertSame(response1, cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db1", ip -> response1));
74+
assertSame(response2, cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db2", ip -> response2));
75+
assertSame(response1, cache.get(projectId, "127.0.0.1", "path/to/db1"));
76+
assertSame(response2, cache.get(projectId, "127.0.0.1", "path/to/db2"));
77+
}
78+
79+
public void testCacheDoesNotCollideForDifferentProjects() {
80+
GeoIpCache cache = new GeoIpCache(2);
81+
AbstractResponse response1 = mock(AbstractResponse.class);
82+
AbstractResponse response2 = mock(AbstractResponse.class);
83+
84+
ProjectId projectId1 = randomUniqueProjectId();
85+
ProjectId projectId2 = randomUniqueProjectId();
86+
assertSame(response1, cache.putIfAbsent(projectId1, "127.0.0.1", "path/to/db1", ip -> response1));
87+
assertSame(response2, cache.putIfAbsent(projectId2, "127.0.0.1", "path/to/db1", ip -> response2));
88+
assertSame(response1, cache.get(projectId1, "127.0.0.1", "path/to/db1"));
89+
assertSame(response2, cache.get(projectId2, "127.0.0.1", "path/to/db1"));
7290
}
7391

7492
public void testThrowsFunctionsException() {
7593
GeoIpCache cache = new GeoIpCache(1);
94+
ProjectId projectId = randomProjectIdOrDefault();
7695
IllegalArgumentException ex = expectThrows(
7796
IllegalArgumentException.class,
78-
() -> cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> {
97+
() -> cache.putIfAbsent(projectId, "127.0.0.1", "path/to/db", ip -> {
7998
throw new IllegalArgumentException("bad");
8099
})
81100
);
@@ -92,19 +111,20 @@ public void testGetCacheStats() {
92111
final AtomicLong testNanoTime = new AtomicLong(0);
93112
// We use a relative time provider that increments 1ms every time it is called. So each operation appears to take 1ms
94113
GeoIpCache cache = new GeoIpCache(maxCacheSize, () -> testNanoTime.addAndGet(TimeValue.timeValueMillis(1).getNanos()));
114+
ProjectId projectId = randomProjectIdOrDefault();
95115
AbstractResponse response = mock(AbstractResponse.class);
96116
String databasePath = "path/to/db1";
97117
String key1 = "127.0.0.1";
98118
String key2 = "127.0.0.2";
99119
String key3 = "127.0.0.3";
100120

101-
cache.putIfAbsent(key1, databasePath, ip -> response); // cache miss
102-
cache.putIfAbsent(key2, databasePath, ip -> response); // cache miss
103-
cache.putIfAbsent(key1, databasePath, ip -> response); // cache hit
104-
cache.putIfAbsent(key1, databasePath, ip -> response); // cache hit
105-
cache.putIfAbsent(key1, databasePath, ip -> response); // cache hit
106-
cache.putIfAbsent(key3, databasePath, ip -> response); // cache miss, key2 will be evicted
107-
cache.putIfAbsent(key2, databasePath, ip -> response); // cache miss, key1 will be evicted
121+
cache.putIfAbsent(projectId, key1, databasePath, ip -> response); // cache miss
122+
cache.putIfAbsent(projectId, key2, databasePath, ip -> response); // cache miss
123+
cache.putIfAbsent(projectId, key1, databasePath, ip -> response); // cache hit
124+
cache.putIfAbsent(projectId, key1, databasePath, ip -> response); // cache hit
125+
cache.putIfAbsent(projectId, key1, databasePath, ip -> response); // cache hit
126+
cache.putIfAbsent(projectId, key3, databasePath, ip -> response); // cache miss, key2 will be evicted
127+
cache.putIfAbsent(projectId, key2, databasePath, ip -> response); // cache miss, key1 will be evicted
108128
CacheStats cacheStats = cache.getCacheStats();
109129
assertThat(cacheStats.count(), equalTo(maxCacheSize));
110130
assertThat(cacheStats.hits(), equalTo(3L));
@@ -115,4 +135,28 @@ public void testGetCacheStats() {
115135
// There are 4 misses. Each is made up of a cache query, and a database query, each being 1ms:
116136
assertThat(cacheStats.missesTimeInMillis(), equalTo(8L));
117137
}
138+
139+
public void testPurgeCacheEntriesForDatabase() {
140+
GeoIpCache cache = new GeoIpCache(100);
141+
ProjectId projectId1 = randomUniqueProjectId();
142+
ProjectId projectId2 = randomUniqueProjectId();
143+
String databasePath1 = "path/to/db1";
144+
String databasePath2 = "path/to/db2";
145+
String ip1 = "127.0.0.1";
146+
String ip2 = "127.0.0.2";
147+
148+
AbstractResponse response = mock(AbstractResponse.class);
149+
cache.putIfAbsent(projectId1, ip1, databasePath1, ip -> response); // cache miss
150+
cache.putIfAbsent(projectId1, ip2, databasePath1, ip -> response); // cache miss
151+
cache.putIfAbsent(projectId2, ip1, databasePath1, ip -> response); // cache miss
152+
cache.putIfAbsent(projectId1, ip1, databasePath2, ip -> response); // cache miss
153+
cache.purgeCacheEntriesForDatabase(projectId1, PathUtils.get(databasePath1));
154+
// should have purged entries for projectId1 and databasePath1...
155+
assertNull(cache.get(projectId1, ip1, databasePath1));
156+
assertNull(cache.get(projectId1, ip2, databasePath1));
157+
// ...but left the one for projectId2...
158+
assertSame(response, cache.get(projectId2, ip1, databasePath1));
159+
// ...and for databasePath2:
160+
assertSame(response, cache.get(projectId1, ip1, databasePath2));
161+
}
118162
}

0 commit comments

Comments
 (0)