From 2ca311cc1b1c62d7fc6f1eeaa022d679a404c854 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Tue, 10 Jun 2025 15:50:51 +0100 Subject: [PATCH 01/19] [WIP] Add option for byte-based capacity for geoip cache This adds the option to have a geoip cache with a byte-based rather than a count-based capacity. --- .../ingest/geoip/DatabaseNodeServiceIT.java | 2 +- ...gDatabasesWhilePerformingGeoLookupsIT.java | 2 +- .../geoip/DatabaseReaderLazyLoader.java | 5 +- .../ingest/geoip/GeoIpCache.java | 66 ++++++++++--- .../ingest/geoip/IngestGeoIpPlugin.java | 2 +- .../ingest/geoip/IpDataLookup.java | 2 +- .../ingest/geoip/IpDatabase.java | 6 +- .../ingest/geoip/MaxmindIpDataLookups.java | 93 ++++++++++++++----- .../ingest/geoip/ConfigDatabasesTests.java | 12 +-- .../geoip/DatabaseNodeServiceTests.java | 2 +- .../ingest/geoip/GeoIpCacheTests.java | 33 +++---- .../geoip/GeoIpProcessorFactoryTests.java | 10 +- .../ingest/geoip/GeoIpProcessorTests.java | 2 +- .../ingest/geoip/GeoIpTestUtils.java | 16 +++- .../geoip/IpinfoIpDataLookupsTests.java | 2 +- .../geoip/MaxmindIpDataLookupsTests.java | 2 +- 16 files changed, 179 insertions(+), 78 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index 7331afdbf585a..9509c483f56c3 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -96,7 +96,7 @@ private void assertValidDatabase(DatabaseNodeService databaseNodeService, String IpDatabase database = databaseNodeService.getDatabase(databaseFileName); assertNotNull(database); assertThat(database.getDatabaseType(), equalTo(databaseType)); - CountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry); + CountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry).result(); assertNotNull(countryResponse); Country country = countryResponse.getCountry(); assertNotNull(country); diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java index 7c92636d3420a..8881f675a8d22 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java @@ -195,7 +195,7 @@ public void test() throws Exception { private static DatabaseNodeService createRegistry(Path geoIpConfigDir, Path geoIpTmpDir, ClusterService clusterService) throws IOException { - GeoIpCache cache = new GeoIpCache(0); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(0); ConfigDatabases configDatabases = new ConfigDatabases(geoIpConfigDir, cache); copyDefaultDatabases(geoIpConfigDir, configDatabases); DatabaseNodeService databaseNodeService = new DatabaseNodeService( diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index fb4fadf043b05..d2858ba36af62 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -105,7 +105,10 @@ int current() { @Override @Nullable - public RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { + public RESPONSE getResponse( + String ipAddress, + CheckedBiFunction responseProvider + ) { return cache.putIfAbsent(ipAddress, cachedDatabasePathToString, ip -> { try { return responseProvider.apply(get(), ipAddress); diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index d9c9c3aaf3266..11082a34a55a6 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -10,8 +10,10 @@ import com.maxmind.db.NodeCache; +import org.apache.lucene.util.RamUsageEstimator; import org.elasticsearch.common.cache.Cache; import org.elasticsearch.common.cache.CacheBuilder; +import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.core.TimeValue; import org.elasticsearch.ingest.geoip.stats.CacheStats; @@ -28,13 +30,48 @@ */ public final class GeoIpCache { + public interface CacheableValue { + + // TODO PETE: Remove this default implementation and implement in all implementing classes instead + default long sizeInBytes() { + return 0; + } + } + + static GeoIpCache createGeoIpCacheWithMaxCount(long maxSize) { + if (maxSize < 0) { + throw new IllegalArgumentException("geoip max cache size must be 0 or greater"); + } + return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); + } + + // TODO PETE: Add tests for this + // TODO PETE: Make plugin use this instead of the other factory method when the settings require it + static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { + if (maxByteSize.getBytes() < 0) { + throw new IllegalArgumentException("geoip max cache size in bytes must be 0 or greater"); + } + return new GeoIpCache( + System::nanoTime, + CacheBuilder.builder() + .setMaximumWeight(maxByteSize.getBytes()) + .weigher((key, value) -> key.sizeInBytes() + value.sizeInBytes()) + .build() + ); + } + + // package private for testing + static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize, LongSupplier relativeNanoTimeProvider) { + return new GeoIpCache(relativeNanoTimeProvider, CacheBuilder.builder().setMaximumWeight(maxSize).build()); + } + /** * Internal-only sentinel object for recording that a result from the geoip database was null (i.e. there was no result). By caching * this no-result we can distinguish between something not being in the cache because we haven't searched for that data yet, versus * something not being in the cache because the data doesn't exist in the database. */ // visible for testing - static final Object NO_RESULT = new Object() { + static final CacheableValue NO_RESULT = new CacheableValue() { @Override public String toString() { return "NO_RESULT"; @@ -42,30 +79,22 @@ public String toString() { }; private final LongSupplier relativeNanoTimeProvider; - private final Cache cache; + private final Cache cache; private final AtomicLong hitsTimeInNanos = new AtomicLong(0); private final AtomicLong missesTimeInNanos = new AtomicLong(0); - // package private for testing - GeoIpCache(long maxSize, LongSupplier relativeNanoTimeProvider) { - if (maxSize < 0) { - throw new IllegalArgumentException("geoip max cache size must be 0 or greater"); - } + private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { this.relativeNanoTimeProvider = relativeNanoTimeProvider; - this.cache = CacheBuilder.builder().setMaximumWeight(maxSize).build(); - } - - GeoIpCache(long maxSize) { - this(maxSize, System::nanoTime); + this.cache = cache; } @SuppressWarnings("unchecked") - RESPONSE putIfAbsent(String ip, String databasePath, Function retrieveFunction) { + RESPONSE putIfAbsent(String ip, String databasePath, Function retrieveFunction) { // can't use cache.computeIfAbsent due to the elevated permissions for the jackson (run via the cache loader) CacheKey cacheKey = new CacheKey(ip, databasePath); long cacheStart = relativeNanoTimeProvider.getAsLong(); // intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition. - Object response = cache.get(cacheKey); + CacheableValue response = cache.get(cacheKey); long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart; // populate the cache for this key, if necessary @@ -135,5 +164,12 @@ public CacheStats getCacheStats() { * 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 * IP may be in both with different values and we need to cache both. */ - private record CacheKey(String ip, String databasePath) {} + private record CacheKey(String ip, String databasePath) { + + private static final long BASE_BYTES = RamUsageEstimator.shallowSizeOfInstance(CacheKey.class); + + public long sizeInBytes() { + return BASE_BYTES + RamUsageEstimator.sizeOf(ip) + RamUsageEstimator.sizeOf(databasePath); + } + } } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java index 68aba5445e2af..c738a33ddf555 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java @@ -120,7 +120,7 @@ public Map getProcessors(Processor.Parameters paramet ingestService.set(parameters.ingestService); long cacheSize = CACHE_SIZE.get(parameters.env.settings()); - GeoIpCache geoIpCache = new GeoIpCache(cacheSize); + GeoIpCache geoIpCache = GeoIpCache.createGeoIpCacheWithMaxCount(cacheSize); DatabaseNodeService registry = new DatabaseNodeService( parameters.env, parameters.client, diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java index 1f1e04274ac25..b426ee3a39d87 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java @@ -34,5 +34,5 @@ interface IpDataLookup { * as a network for which the record applies. Having a helper record prevents each individual response record from needing to * track these bits of information. */ - record Result(T result, String ip, String network) {} + record Result(T result, String ip, String network) implements GeoIpCache.CacheableValue {} } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java index db1ffc1c682b8..c3faa10c20010 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java @@ -33,10 +33,12 @@ public interface IpDatabase extends AutoCloseable { * @param ipAddress the address to lookup * @param responseProvider a method for extracting a response from a {@link Reader}, usually this will be a method reference * @return a possibly-null response - * @param the type of response that will be returned */ @Nullable - RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider); + RESPONSE getResponse( + String ipAddress, + CheckedBiFunction responseProvider + ); /** * Releases the current database object. Called after processing a single document. Databases should be closed or returned to a diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 196137ce99c4c..c57383d64c04f 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -109,7 +109,23 @@ static Function, IpDataLookup> getMaxmindLookup(final Dat }; } - static class AnonymousIp extends AbstractBase { + static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements GeoIpCache.CacheableValue { + + CacheableAnonymousIpResponse(AnonymousIpResponse response) { + super( + response.getIpAddress(), + response.isAnonymous(), + response.isAnonymousVpn(), + response.isHostingProvider(), + response.isPublicProxy(), + response.isResidentialProxy(), + response.isTorExitNode(), + response.getNetwork() + ); + } + } + + static class AnonymousIp extends AbstractBase { AnonymousIp(final Set properties) { super( properties, @@ -119,12 +135,12 @@ static class AnonymousIp extends AbstractBase transform(final AnonymousIpResponse response) { + protected Map transform(final CacheableAnonymousIpResponse response) { boolean isHostingProvider = response.isHostingProvider(); boolean isTorExitNode = response.isTorExitNode(); boolean isAnonymousVpn = response.isAnonymousVpn(); @@ -160,18 +176,30 @@ protected Map transform(final AnonymousIpResponse response) { } } - static class Asn extends AbstractBase { + static class CacheableAsnResponse extends AsnResponse implements GeoIpCache.CacheableValue { + + CacheableAsnResponse(AsnResponse response) { + super( + response.getAutonomousSystemNumber(), + response.getAutonomousSystemOrganization(), + response.getIpAddress(), + response.getNetwork() + ); + } + } + + static class Asn extends AbstractBase { Asn(Set properties) { super(properties, AsnResponse.class, (response, ipAddress, network, locales) -> new AsnResponse(response, ipAddress, network)); } @Override - protected AsnResponse cacheableRecord(AsnResponse response) { - return response; + protected CacheableAsnResponse cacheableRecord(AsnResponse response) { + return new CacheableAsnResponse(response); } @Override - protected Map transform(final AsnResponse response) { + protected Map transform(final CacheableAsnResponse response) { Long asn = response.getAutonomousSystemNumber(); String organizationName = response.getAutonomousSystemOrganization(); Network network = response.getNetwork(); @@ -354,7 +382,14 @@ protected Map transform(final Result resu } } - static class ConnectionType extends AbstractBase { + static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements GeoIpCache.CacheableValue { + + CacheableConnectionTypeResponse(ConnectionTypeResponse response) { + super(response.getConnectionType(), response.getIpAddress(), response.getNetwork()); + } + } + + static class ConnectionType extends AbstractBase { ConnectionType(final Set properties) { super( properties, @@ -364,12 +399,12 @@ static class ConnectionType extends AbstractBase transform(final ConnectionTypeResponse response) { + protected Map transform(final CacheableConnectionTypeResponse response) { ConnectionTypeResponse.ConnectionType connectionType = response.getConnectionType(); Map data = new HashMap<>(); @@ -479,7 +514,14 @@ protected Map transform(final Result r } } - static class Domain extends AbstractBase { + static class CacheableDomainResponse extends DomainResponse implements GeoIpCache.CacheableValue { + + CacheableDomainResponse(DomainResponse response) { + super(response.getDomain(), response.getIpAddress(), response.getNetwork()); + } + } + + static class Domain extends AbstractBase { Domain(final Set properties) { super( properties, @@ -489,12 +531,12 @@ static class Domain extends AbstractBase { } @Override - protected DomainResponse cacheableRecord(DomainResponse response) { - return response; + protected CacheableDomainResponse cacheableRecord(DomainResponse response) { + return new CacheableDomainResponse(response); } @Override - protected Map transform(final DomainResponse response) { + protected Map transform(final CacheableDomainResponse response) { String domain = response.getDomain(); Map data = new HashMap<>(); @@ -784,18 +826,25 @@ protected Map transform(final Result { + static class CacheableIspResponse extends IspResponse implements GeoIpCache.CacheableValue { + + CacheableIspResponse(IspResponse response) { + super(response, response.getIpAddress(), response.getNetwork()); + } + } + + static class Isp extends AbstractBase { Isp(final Set properties) { super(properties, IspResponse.class, (response, ipAddress, network, locales) -> new IspResponse(response, ipAddress, network)); } @Override - protected IspResponse cacheableRecord(IspResponse response) { - return response; + protected CacheableIspResponse cacheableRecord(IspResponse response) { + return new CacheableIspResponse(response); } @Override - protected Map transform(final IspResponse response) { + protected Map transform(final CacheableIspResponse response) { String isp = response.getIsp(); String ispOrganization = response.getOrganization(); String mobileNetworkCode = response.getMobileNetworkCode(); @@ -867,7 +916,9 @@ private interface ResponseBuilder { * * @param the intermediate type of {@link AbstractResponse} */ - private abstract static class AbstractBase implements IpDataLookup { + private abstract static class AbstractBase + implements + IpDataLookup { protected final Set properties; protected final Class clazz; diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java index 7f38a37b43edf..f5d8617019962 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java @@ -50,7 +50,7 @@ public void cleanup() { public void testLocalDatabasesEmptyConfig() throws Exception { Path configDir = createTempDir(); - ConfigDatabases configDatabases = new ConfigDatabases(configDir, new GeoIpCache(0)); + ConfigDatabases configDatabases = new ConfigDatabases(configDir, GeoIpCache.createGeoIpCacheWithMaxCount(0)); configDatabases.initialize(resourceWatcherService); assertThat(configDatabases.getConfigDatabases(), anEmptyMap()); @@ -64,7 +64,7 @@ public void testDatabasesConfigDir() throws Exception { copyDatabase("GeoIP2-City-Test.mmdb", configDir.resolve("GeoIP2-City.mmdb")); copyDatabase("GeoLite2-City-Test.mmdb", configDir.resolve("GeoLite2-City.mmdb")); - ConfigDatabases configDatabases = new ConfigDatabases(configDir, new GeoIpCache(0)); + ConfigDatabases configDatabases = new ConfigDatabases(configDir, GeoIpCache.createGeoIpCacheWithMaxCount(0)); configDatabases.initialize(resourceWatcherService); assertThat(configDatabases.getConfigDatabases().size(), equalTo(2)); @@ -77,7 +77,7 @@ public void testDatabasesConfigDir() throws Exception { public void testDatabasesDynamicUpdateConfigDir() throws Exception { Path configDir = prepareConfigDir(); - ConfigDatabases configDatabases = new ConfigDatabases(configDir, new GeoIpCache(0)); + ConfigDatabases configDatabases = new ConfigDatabases(configDir, GeoIpCache.createGeoIpCacheWithMaxCount(0)); configDatabases.initialize(resourceWatcherService); { assertThat(configDatabases.getConfigDatabases().size(), equalTo(3)); @@ -117,7 +117,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { Path configDir = createTempDir(); copyDatabase("GeoLite2-City.mmdb", configDir); - GeoIpCache cache = new GeoIpCache(1000); // real cache to test purging of entries upon a reload + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); // real cache to test purging of entries upon a reload ConfigDatabases configDatabases = new ConfigDatabases(configDir, cache); configDatabases.initialize(resourceWatcherService); { @@ -126,7 +126,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); + CityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); assertThat(cityResponse.getCity().getName(), equalTo("Tumba")); assertThat(cache.count(), equalTo(1)); } @@ -138,7 +138,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); + CityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); assertThat(cityResponse.getCity().getName(), equalTo("Linköping")); assertThat(cache.count(), equalTo(1)); }); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceTests.java index 41a38b6f4e3ef..f5d372005f07d 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceTests.java @@ -120,7 +120,7 @@ public class DatabaseNodeServiceTests extends ESTestCase { @Before public void setup() throws IOException { final Path geoIpConfigDir = createTempDir(); - GeoIpCache cache = new GeoIpCache(1000); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); ConfigDatabases configDatabases = new ConfigDatabases(geoIpConfigDir, cache); copyDefaultDatabases(geoIpConfigDir, configDatabases); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index 0c92aca882913..3559b69831200 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -9,8 +9,6 @@ package org.elasticsearch.ingest.geoip; -import com.maxmind.geoip2.model.AbstractResponse; - import org.elasticsearch.core.TimeValue; import org.elasticsearch.ingest.geoip.stats.CacheStats; import org.elasticsearch.test.ESTestCase; @@ -25,12 +23,12 @@ public class GeoIpCacheTests extends ESTestCase { public void testCachesAndEvictsResults() { - GeoIpCache cache = new GeoIpCache(1); - AbstractResponse response1 = mock(AbstractResponse.class); - AbstractResponse response2 = mock(AbstractResponse.class); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); + GeoIpCache.CacheableValue response1 = mock(GeoIpCache.CacheableValue.class); + GeoIpCache.CacheableValue response2 = mock(GeoIpCache.CacheableValue.class); // add a key - AbstractResponse cachedResponse = cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1); + GeoIpCache.CacheableValue cachedResponse = cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1); assertSame(cachedResponse, response1); assertSame(cachedResponse, cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1)); assertSame(cachedResponse, cache.get("127.0.0.1", "path/to/db")); @@ -44,14 +42,14 @@ public void testCachesAndEvictsResults() { } public void testCachesNoResult() { - GeoIpCache cache = new GeoIpCache(1); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); final AtomicInteger count = new AtomicInteger(0); - Function countAndReturnNull = (ip) -> { + Function countAndReturnNull = (ip) -> { count.incrementAndGet(); return null; }; - AbstractResponse response = cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull); + GeoIpCache.CacheableValue response = cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull); assertNull(response); assertNull(cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull)); assertEquals(1, count.get()); @@ -61,9 +59,9 @@ public void testCachesNoResult() { } public void testCacheKey() { - GeoIpCache cache = new GeoIpCache(2); - AbstractResponse response1 = mock(AbstractResponse.class); - AbstractResponse response2 = mock(AbstractResponse.class); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(2); + GeoIpCache.CacheableValue response1 = mock(GeoIpCache.CacheableValue.class); + GeoIpCache.CacheableValue response2 = mock(GeoIpCache.CacheableValue.class); assertSame(response1, cache.putIfAbsent("127.0.0.1", "path/to/db1", ip -> response1)); assertSame(response2, cache.putIfAbsent("127.0.0.1", "path/to/db2", ip -> response2)); @@ -72,7 +70,7 @@ public void testCacheKey() { } public void testThrowsFunctionsException() { - GeoIpCache cache = new GeoIpCache(1); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); IllegalArgumentException ex = expectThrows( IllegalArgumentException.class, () -> cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> { @@ -83,7 +81,7 @@ public void testThrowsFunctionsException() { } public void testInvalidInit() { - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new GeoIpCache(-1)); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> GeoIpCache.createGeoIpCacheWithMaxCount(-1)); assertEquals("geoip max cache size must be 0 or greater", ex.getMessage()); } @@ -91,8 +89,11 @@ public void testGetCacheStats() { final long maxCacheSize = 2; final AtomicLong testNanoTime = new AtomicLong(0); // We use a relative time provider that increments 1ms every time it is called. So each operation appears to take 1ms - GeoIpCache cache = new GeoIpCache(maxCacheSize, () -> testNanoTime.addAndGet(TimeValue.timeValueMillis(1).getNanos())); - AbstractResponse response = mock(AbstractResponse.class); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCountAndCustomTimeProvider( + maxCacheSize, + () -> testNanoTime.addAndGet(TimeValue.timeValueMillis(1).getNanos()) + ); + GeoIpCache.CacheableValue response = mock(GeoIpCache.CacheableValue.class); String databasePath = "path/to/db1"; String key1 = "127.0.0.1"; String key2 = "127.0.0.2"; diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorFactoryTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorFactoryTests.java index 65b4c52a29e0f..5ef473a66e8bf 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorFactoryTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorFactoryTests.java @@ -72,8 +72,8 @@ public void loadDatabaseReaders() throws IOException { Files.createDirectories(geoIpConfigDir); Client client = mock(Client.class); - GeoIpCache cache = new GeoIpCache(1000); - configDatabases = new ConfigDatabases(geoIpConfigDir, new GeoIpCache(1000)); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); + configDatabases = new ConfigDatabases(geoIpConfigDir, GeoIpCache.createGeoIpCacheWithMaxCount(1000)); copyDefaultDatabases(geoIpConfigDir, configDatabases); geoipTmpDir = createTempDir(); clusterService = mock(ClusterService.class); @@ -366,7 +366,7 @@ public void testLazyLoading() throws Exception { final Path configDir = createTempDir(); final Path geoIpConfigDir = configDir.resolve("ingest-geoip"); Files.createDirectories(geoIpConfigDir); - GeoIpCache cache = new GeoIpCache(1000); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); ConfigDatabases configDatabases = new ConfigDatabases(geoIpConfigDir, cache); copyDefaultDatabases(geoIpConfigDir, configDatabases); @@ -428,7 +428,7 @@ public void testLoadingCustomDatabase() throws IOException { final Path configDir = createTempDir(); final Path geoIpConfigDir = configDir.resolve("ingest-geoip"); Files.createDirectories(geoIpConfigDir); - ConfigDatabases configDatabases = new ConfigDatabases(geoIpConfigDir, new GeoIpCache(1000)); + ConfigDatabases configDatabases = new ConfigDatabases(geoIpConfigDir, GeoIpCache.createGeoIpCacheWithMaxCount(1000)); copyDefaultDatabases(geoIpConfigDir, configDatabases); // fake the GeoIP2-City database copyDatabase("GeoLite2-City.mmdb", geoIpConfigDir); @@ -441,7 +441,7 @@ public void testLoadingCustomDatabase() throws IOException { ThreadPool threadPool = new TestThreadPool("test"); ResourceWatcherService resourceWatcherService = new ResourceWatcherService(Settings.EMPTY, threadPool); Client client = mock(Client.class); - GeoIpCache cache = new GeoIpCache(1000); + GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); DatabaseNodeService databaseNodeService = new DatabaseNodeService( createTempDir(), client, diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorTests.java index 4548e92239ce1..e77071e9ff5c1 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorTests.java @@ -489,7 +489,7 @@ private DatabaseReaderLazyLoader loader(final String databaseName, final AtomicB final Path path = tmpDir.resolve(last == -1 ? databaseName : databaseName.substring(last + 1)); copyDatabase(databaseName, path); - final GeoIpCache cache = new GeoIpCache(1000); + final GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); return new DatabaseReaderLazyLoader(cache, path, null) { @Override protected void doShutdown() throws IOException { diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 160671fd39001..b32af4bb95717 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -73,10 +73,12 @@ public static void copyDefaultDatabases(final Path directory, ConfigDatabases co *

* Like this: {@code CityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} */ - public static CityResponse getCity(Reader reader, String ip) throws IOException { + public static IpDataLookup.Result getCity(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CityResponse.class); CityResponse data = record.getData(); - return data == null ? null : new CityResponse(data, ip, record.getNetwork(), List.of("en")); + return data == null + ? null + : new IpDataLookup.Result<>(new CityResponse(data, ip, record.getNetwork(), List.of("en")), ip, record.getNetwork().toString()); } /** @@ -85,9 +87,15 @@ public static CityResponse getCity(Reader reader, String ip) throws IOException *

* Like this: {@code CountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} */ - public static CountryResponse getCountry(Reader reader, String ip) throws IOException { + public static IpDataLookup.Result getCountry(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CountryResponse.class); CountryResponse data = record.getData(); - return data == null ? null : new CountryResponse(data, ip, record.getNetwork(), List.of("en")); + return data == null + ? null + : new IpDataLookup.Result<>( + new CountryResponse(data, ip, record.getNetwork(), List.of("en")), + ip, + record.getNetwork().toString() + ); } } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookupsTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookupsTests.java index 5d45ff5d55855..68c79b78ec542 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookupsTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookupsTests.java @@ -558,7 +558,7 @@ private void assertActualResultsMatchReader( private DatabaseReaderLazyLoader loader(final String databaseName) { Path path = tmpDir.resolve(databaseName); copyDatabase("ipinfo/" + databaseName, path); // the ipinfo databases are prefixed on the test classpath - final GeoIpCache cache = new GeoIpCache(1000); + final GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); return new DatabaseReaderLazyLoader(cache, path, null); } } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java index 88cff23cd9b7f..32251dc097043 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookupsTests.java @@ -333,7 +333,7 @@ private void assertExpectedLookupResults(String databaseName, String ip, IpDataL private DatabaseReaderLazyLoader loader(final String databaseName) { Path path = tmpDir.resolve(databaseName); copyDatabase(databaseName, path); - final GeoIpCache cache = new GeoIpCache(1000); + final GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1000); return new DatabaseReaderLazyLoader(cache, path, null); } } From 1b2afaf94894b9b74678a8157ff8b24802faed6d Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 11 Jun 2025 14:08:16 +0100 Subject: [PATCH 02/19] Constrain the generic type parameter of `IpDataLookup.Result` to extend `GeoIpCache.CacheableValue`. This will be needed to implement the `sizeInBytes()` method on `Result`. The code in `GeoIpTestUtils` has to change a little bit, because previously we had `Result` and we don't own `CityResponse` so we can't make it implement the interface (and the same for `CountryResponse`). This change makes the test helper act a bit more like the production code, which is probably a good thing. --- .../ingest/geoip/DatabaseNodeServiceIT.java | 10 +- .../ingest/geoip/IpDataLookup.java | 2 +- .../ingest/geoip/IpinfoIpDataLookups.java | 22 ++--- .../ingest/geoip/MaxmindIpDataLookups.java | 97 +++++++++---------- .../ingest/geoip/ConfigDatabasesTests.java | 11 +-- .../ingest/geoip/GeoIpTestUtils.java | 17 ++-- 6 files changed, 76 insertions(+), 83 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index 9509c483f56c3..adc49bfc04628 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -9,13 +9,11 @@ package org.elasticsearch.ingest.geoip; -import com.maxmind.geoip2.model.CountryResponse; -import com.maxmind.geoip2.record.Country; - import org.elasticsearch.action.admin.indices.flush.FlushRequest; import org.elasticsearch.action.admin.indices.refresh.RefreshRequest; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.common.hash.MessageDigests; +import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCountryResponse; import org.elasticsearch.xcontent.XContentType; import java.io.BufferedInputStream; @@ -96,11 +94,9 @@ private void assertValidDatabase(DatabaseNodeService databaseNodeService, String IpDatabase database = databaseNodeService.getDatabase(databaseFileName); assertNotNull(database); assertThat(database.getDatabaseType(), equalTo(databaseType)); - CountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry).result(); + CacheableCountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry).result(); assertNotNull(countryResponse); - Country country = countryResponse.getCountry(); - assertNotNull(country); - assertThat(country.getName(), equalTo("Sweden")); + assertThat(countryResponse.countryName(), equalTo("Sweden")); } /* diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java index b426ee3a39d87..9319979542a73 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java @@ -34,5 +34,5 @@ interface IpDataLookup { * as a network for which the record applies. Having a helper record prevents each individual response record from needing to * track these bits of information. */ - record Result(T result, String ip, String network) implements GeoIpCache.CacheableValue {} + record Result(T result, String ip, String network) implements GeoIpCache.CacheableValue {} } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index d548fa9bc98ea..ce6da6806283f 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -191,7 +191,7 @@ public record AsnResult( String domain, String name, @Nullable String type // not present in the free asn database - ) { + ) implements GeoIpCache.CacheableValue { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public AsnResult( @@ -210,20 +210,14 @@ public record CountryResult( @MaxMindDbParameter(name = "continent_name") String continentName, @MaxMindDbParameter(name = "country") String country, @MaxMindDbParameter(name = "country_name") String countryName - ) { + ) implements GeoIpCache.CacheableValue { @MaxMindDbConstructor public CountryResult {} } - public record GeolocationResult( - String city, - String country, - Double lat, - Double lng, - String postalCode, - String region, - String timezone - ) { + public record GeolocationResult(String city, String country, Double lat, Double lng, String postalCode, String region, String timezone) + implements + GeoIpCache.CacheableValue { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public GeolocationResult( @@ -241,7 +235,9 @@ public GeolocationResult( } } - public record PrivacyDetectionResult(Boolean hosting, Boolean proxy, Boolean relay, String service, Boolean tor, Boolean vpn) { + public record PrivacyDetectionResult(Boolean hosting, Boolean proxy, Boolean relay, String service, Boolean tor, Boolean vpn) + implements + GeoIpCache.CacheableValue { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public PrivacyDetectionResult( @@ -466,7 +462,7 @@ protected Map transform(final Result res * * @param the record type that will be wrapped and returned */ - private abstract static class AbstractBase implements IpDataLookup { + private abstract static class AbstractBase implements IpDataLookup { protected final Set properties; protected final Class clazz; diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index c57383d64c04f..611762e1f6fba 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -229,7 +229,7 @@ protected Map transform(final CacheableAsnResponse response) { } } - record CacheableCityResponse( + public record CacheableCityResponse( Boolean isInEuropeanUnion, String countryIsoCode, String countryName, @@ -246,7 +246,29 @@ record CacheableCityResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) {} + ) implements GeoIpCache.CacheableValue { + + public static CacheableCityResponse from(CityResponse response) { + return new CacheableCityResponse( + MaxmindIpDataLookups.isInEuropeanUnion(response.getCountry()), + response.getCountry().getIsoCode(), + response.getCountry().getName(), + response.getContinent().getCode(), + response.getContinent().getName(), + MaxmindIpDataLookups.regionIsoCode(response.getCountry(), response.getMostSpecificSubdivision()), + response.getMostSpecificSubdivision().getName(), + response.getCity().getName(), + response.getLocation().getTimeZone(), + response.getLocation().getLatitude(), + response.getLocation().getLongitude(), + response.getLocation().getAccuracyRadius(), + response.getPostal().getCode(), + MaxmindIpDataLookups.isInEuropeanUnion(response.getRegisteredCountry()), + response.getRegisteredCountry().getIsoCode(), + response.getRegisteredCountry().getName() + ); + } + } static class City extends AbstractBase> { City(final Set properties) { @@ -255,36 +277,10 @@ static class City extends AbstractBase cacheableRecord(CityResponse response) { - final com.maxmind.geoip2.record.Country country = response.getCountry(); - final Continent continent = response.getContinent(); - final Subdivision subdivision = response.getMostSpecificSubdivision(); - final com.maxmind.geoip2.record.City city = response.getCity(); - final Location location = response.getLocation(); - final Postal postal = response.getPostal(); - final com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); - final Traits traits = response.getTraits(); - return new Result<>( - new CacheableCityResponse( - isInEuropeanUnion(country), - country.getIsoCode(), - country.getName(), - continent.getCode(), - continent.getName(), - regionIsoCode(country, subdivision), - subdivision.getName(), - city.getName(), - location.getTimeZone(), - location.getLatitude(), - location.getLongitude(), - location.getAccuracyRadius(), - postal.getCode(), - isInEuropeanUnion(registeredCountry), - registeredCountry.getIsoCode(), - registeredCountry.getName() - ), - traits.getIpAddress(), - traits.getNetwork().toString() + CacheableCityResponse.from(response), + response.getTraits().getIpAddress(), + response.getTraits().getNetwork().toString() ); } @@ -422,7 +418,7 @@ protected Map transform(final CacheableConnectionTypeResponse re } } - record CacheableCountryResponse( + public record CacheableCountryResponse( Boolean isInEuropeanUnion, String countryIsoCode, String countryName, @@ -431,7 +427,21 @@ record CacheableCountryResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) {} + ) implements GeoIpCache.CacheableValue { + + public static CacheableCountryResponse from(CountryResponse response) { + return new CacheableCountryResponse( + MaxmindIpDataLookups.isInEuropeanUnion(response.getCountry()), + response.getCountry().getIsoCode(), + response.getCountry().getName(), + response.getContinent().getCode(), + response.getContinent().getName(), + MaxmindIpDataLookups.isInEuropeanUnion(response.getRegisteredCountry()), + response.getRegisteredCountry().getIsoCode(), + response.getRegisteredCountry().getName() + ); + } + } static class Country extends AbstractBase> { Country(final Set properties) { @@ -440,23 +450,10 @@ static class Country extends AbstractBase cacheableRecord(CountryResponse response) { - final com.maxmind.geoip2.record.Country country = response.getCountry(); - final Continent continent = response.getContinent(); - final com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); - final Traits traits = response.getTraits(); return new Result<>( - new CacheableCountryResponse( - isInEuropeanUnion(country), - country.getIsoCode(), - country.getName(), - continent.getCode(), - continent.getName(), - isInEuropeanUnion(registeredCountry), - registeredCountry.getIsoCode(), - registeredCountry.getName() - ), - traits.getIpAddress(), - traits.getNetwork().toString() + CacheableCountryResponse.from(response), + response.getTraits().getIpAddress(), + response.getTraits().getNetwork().toString() ); } @@ -589,7 +586,7 @@ record CacheableEnterpriseResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) {} + ) implements GeoIpCache.CacheableValue {} static class Enterprise extends AbstractBase> { Enterprise(final Set properties) { diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java index f5d8617019962..2f3e7f5e4476a 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java @@ -9,10 +9,9 @@ package org.elasticsearch.ingest.geoip; -import com.maxmind.geoip2.model.CityResponse; - import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.TimeValue; +import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCityResponse; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; @@ -126,8 +125,8 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); - assertThat(cityResponse.getCity().getName(), equalTo("Tumba")); + CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); + assertThat(cityResponse.cityName(), equalTo("Tumba")); assertThat(cache.count(), equalTo(1)); } @@ -138,8 +137,8 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); - assertThat(cityResponse.getCity().getName(), equalTo("Linköping")); + CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); + assertThat(cityResponse.cityName(), equalTo("Linköping")); assertThat(cache.count(), equalTo(1)); }); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index b32af4bb95717..c0f1e5e0aee05 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -71,29 +71,34 @@ public static void copyDefaultDatabases(final Path directory, ConfigDatabases co * A static city-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in * tests. *

- * Like this: {@code CityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} + * Like this: {@code CacheableCityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} */ - public static IpDataLookup.Result getCity(Reader reader, String ip) throws IOException { + public static IpDataLookup.Result getCity(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CityResponse.class); CityResponse data = record.getData(); return data == null ? null - : new IpDataLookup.Result<>(new CityResponse(data, ip, record.getNetwork(), List.of("en")), ip, record.getNetwork().toString()); + : new IpDataLookup.Result<>( + MaxmindIpDataLookups.CacheableCityResponse.from(new CityResponse(data, ip, record.getNetwork(), List.of("en"))), + ip, + record.getNetwork().toString() + ); } /** * A static country-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in * tests. *

- * Like this: {@code CountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} + * Like this: {@code CacheableCountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} */ - public static IpDataLookup.Result getCountry(Reader reader, String ip) throws IOException { + public static IpDataLookup.Result getCountry(Reader reader, String ip) + throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CountryResponse.class); CountryResponse data = record.getData(); return data == null ? null : new IpDataLookup.Result<>( - new CountryResponse(data, ip, record.getNetwork(), List.of("en")), + MaxmindIpDataLookups.CacheableCountryResponse.from(new CountryResponse(data, ip, record.getNetwork(), List.of("en"))), ip, record.getNetwork().toString() ); From ea4d2f0ab7b1885e8588d4c1ee0979d13bc159c1 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 11 Jun 2025 14:30:48 +0100 Subject: [PATCH 03/19] Revert a change to the test code from the original commit, which is no longer necessary due to the next commit --- .../ingest/geoip/DatabaseNodeServiceIT.java | 2 +- .../ingest/geoip/ConfigDatabasesTests.java | 4 ++-- .../ingest/geoip/GeoIpTestUtils.java | 17 ++++------------- 3 files changed, 7 insertions(+), 16 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index adc49bfc04628..ed55bb91df591 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -94,7 +94,7 @@ private void assertValidDatabase(DatabaseNodeService databaseNodeService, String IpDatabase database = databaseNodeService.getDatabase(databaseFileName); assertNotNull(database); assertThat(database.getDatabaseType(), equalTo(databaseType)); - CacheableCountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry).result(); + CacheableCountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry); assertNotNull(countryResponse); assertThat(countryResponse.countryName(), equalTo("Sweden")); } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java index 2f3e7f5e4476a..1e5555d9cfd8c 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java @@ -125,7 +125,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); + CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Tumba")); assertThat(cache.count(), equalTo(1)); } @@ -137,7 +137,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity).result(); + CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Linköping")); assertThat(cache.count(), equalTo(1)); }); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index c0f1e5e0aee05..3497002968d92 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -73,16 +73,12 @@ public static void copyDefaultDatabases(final Path directory, ConfigDatabases co *

* Like this: {@code CacheableCityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} */ - public static IpDataLookup.Result getCity(Reader reader, String ip) throws IOException { + public static MaxmindIpDataLookups.CacheableCityResponse getCity(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CityResponse.class); CityResponse data = record.getData(); return data == null ? null - : new IpDataLookup.Result<>( - MaxmindIpDataLookups.CacheableCityResponse.from(new CityResponse(data, ip, record.getNetwork(), List.of("en"))), - ip, - record.getNetwork().toString() - ); + : MaxmindIpDataLookups.CacheableCityResponse.from(new CityResponse(data, ip, record.getNetwork(), List.of("en"))); } /** @@ -91,16 +87,11 @@ public static IpDataLookup.Result ge *

* Like this: {@code CacheableCountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} */ - public static IpDataLookup.Result getCountry(Reader reader, String ip) - throws IOException { + public static MaxmindIpDataLookups.CacheableCountryResponse getCountry(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CountryResponse.class); CountryResponse data = record.getData(); return data == null ? null - : new IpDataLookup.Result<>( - MaxmindIpDataLookups.CacheableCountryResponse.from(new CountryResponse(data, ip, record.getNetwork(), List.of("en"))), - ip, - record.getNetwork().toString() - ); + : MaxmindIpDataLookups.CacheableCountryResponse.from(new CountryResponse(data, ip, record.getNetwork(), List.of("en"))); } } From 9f6b9953142e00fa6bcd23fe3355ac3d82b43f7f Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 11 Jun 2025 14:34:33 +0100 Subject: [PATCH 04/19] Minor cleanup --- .../ingest/geoip/GeoIpTestUtils.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 3497002968d92..72d9e1f59ac5c 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -17,6 +17,8 @@ import org.elasticsearch.common.CheckedBiFunction; import org.elasticsearch.common.network.InetAddresses; import org.elasticsearch.core.SuppressForbidden; +import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCityResponse; +import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCountryResponse; import java.io.FileNotFoundException; import java.io.IOException; @@ -42,7 +44,7 @@ private static boolean isDirectory(final Path path) { return path.toFile().isDirectory(); } - public static void copyDatabase(final String databaseName, final Path destination) { + static void copyDatabase(final String databaseName, final Path destination) { try (InputStream is = GeoIpTestUtils.class.getResourceAsStream("/" + databaseName)) { if (is == null) { throw new FileNotFoundException("Resource [" + databaseName + "] not found in classpath"); @@ -54,13 +56,13 @@ public static void copyDatabase(final String databaseName, final Path destinatio } } - public static void copyDefaultDatabases(final Path directory) { + static void copyDefaultDatabases(final Path directory) { for (final String database : DEFAULT_DATABASES) { copyDatabase(database, directory); } } - public static void copyDefaultDatabases(final Path directory, ConfigDatabases configDatabases) { + static void copyDefaultDatabases(final Path directory, ConfigDatabases configDatabases) { for (final String database : DEFAULT_DATABASES) { copyDatabase(database, directory); configDatabases.updateDatabase(directory.resolve(database), true); @@ -73,12 +75,10 @@ public static void copyDefaultDatabases(final Path directory, ConfigDatabases co *

* Like this: {@code CacheableCityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} */ - public static MaxmindIpDataLookups.CacheableCityResponse getCity(Reader reader, String ip) throws IOException { + public static CacheableCityResponse getCity(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CityResponse.class); CityResponse data = record.getData(); - return data == null - ? null - : MaxmindIpDataLookups.CacheableCityResponse.from(new CityResponse(data, ip, record.getNetwork(), List.of("en"))); + return data == null ? null : CacheableCityResponse.from(new CityResponse(data, ip, record.getNetwork(), List.of("en"))); } /** @@ -87,11 +87,9 @@ public static MaxmindIpDataLookups.CacheableCityResponse getCity(Reader reader, *

* Like this: {@code CacheableCountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} */ - public static MaxmindIpDataLookups.CacheableCountryResponse getCountry(Reader reader, String ip) throws IOException { + public static CacheableCountryResponse getCountry(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CountryResponse.class); CountryResponse data = record.getData(); - return data == null - ? null - : MaxmindIpDataLookups.CacheableCountryResponse.from(new CountryResponse(data, ip, record.getNetwork(), List.of("en"))); + return data == null ? null : CacheableCountryResponse.from(new CountryResponse(data, ip, record.getNetwork(), List.of("en"))); } } From 3a92e988bd444cd7948f72b7a5e29c3266159dd3 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 11 Jun 2025 17:10:40 +0100 Subject: [PATCH 05/19] Add unit tests for byte-capacity cache --- .../ingest/geoip/GeoIpCache.java | 10 ++-- .../ingest/geoip/GeoIpCacheTests.java | 49 +++++++++++++++---- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index 11082a34a55a6..1aa36e8b39847 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -45,7 +45,6 @@ static GeoIpCache createGeoIpCacheWithMaxCount(long maxSize) { return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); } - // TODO PETE: Add tests for this // TODO PETE: Make plugin use this instead of the other factory method when the settings require it static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { if (maxByteSize.getBytes() < 0) { @@ -168,8 +167,13 @@ private record CacheKey(String ip, String databasePath) { private static final long BASE_BYTES = RamUsageEstimator.shallowSizeOfInstance(CacheKey.class); - public long sizeInBytes() { - return BASE_BYTES + RamUsageEstimator.sizeOf(ip) + RamUsageEstimator.sizeOf(databasePath); + private long sizeInBytes() { + return keySizeInBytes(ip, databasePath); } } + + // visible for testing + static long keySizeInBytes(String ip, String databasePath) { + return CacheKey.BASE_BYTES + RamUsageEstimator.sizeOf(ip) + RamUsageEstimator.sizeOf(databasePath); + } } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index 3559b69831200..0830fd3160af5 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.ingest.geoip; +import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.core.TimeValue; import org.elasticsearch.ingest.geoip.stats.CacheStats; import org.elasticsearch.test.ESTestCase; @@ -18,17 +19,18 @@ import java.util.function.Function; import static org.hamcrest.Matchers.equalTo; -import static org.mockito.Mockito.mock; public class GeoIpCacheTests extends ESTestCase { - public void testCachesAndEvictsResults() { + private record FakeResponse(long sizeInBytes) implements GeoIpCache.CacheableValue {} + + public void testCachesAndEvictsResults_maxCount() { GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); - GeoIpCache.CacheableValue response1 = mock(GeoIpCache.CacheableValue.class); - GeoIpCache.CacheableValue response2 = mock(GeoIpCache.CacheableValue.class); + FakeResponse response1 = new FakeResponse(0); + FakeResponse response2 = new FakeResponse(0); // add a key - GeoIpCache.CacheableValue cachedResponse = cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1); + FakeResponse cachedResponse = cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1); assertSame(cachedResponse, response1); assertSame(cachedResponse, cache.putIfAbsent("127.0.0.1", "path/to/db", ip -> response1)); assertSame(cachedResponse, cache.get("127.0.0.1", "path/to/db")); @@ -41,15 +43,42 @@ public void testCachesAndEvictsResults() { assertNotSame(response1, cache.get("127.0.0.1", "path/to/db")); } + public void testCachesAndEvictsResults_maxBytes() { + String ip1 = "127.0.0.1"; + String databasePath1 = "path/to/db"; + FakeResponse response1 = new FakeResponse(111); + String ip2 = "127.0.0.2"; + String databasePath2 = "path/to/db"; + FakeResponse response2 = new FakeResponse(222); + long totalSize = GeoIpCache.keySizeInBytes(ip1, databasePath1) + response1.sizeInBytes() + GeoIpCache.keySizeInBytes( + ip2, + databasePath2 + ) + response2.sizeInBytes(); + + GeoIpCache justBigEnoughCache = GeoIpCache.createGeoIpCacheWithMaxBytes(ByteSizeValue.ofBytes(totalSize)); + justBigEnoughCache.putIfAbsent(ip1, databasePath1, ip -> response1); + justBigEnoughCache.putIfAbsent(ip2, databasePath2, ip -> response2); + // Cache is just big enough for both values: + assertSame(response2, justBigEnoughCache.get(ip2, databasePath2)); + assertSame(response1, justBigEnoughCache.get(ip1, databasePath1)); + + GeoIpCache justTooSmallCache = GeoIpCache.createGeoIpCacheWithMaxBytes(ByteSizeValue.ofBytes(totalSize - 1L)); + justTooSmallCache.putIfAbsent(ip1, databasePath1, ip -> response1); + justTooSmallCache.putIfAbsent(ip2, databasePath2, ip -> response2); + // Cache is just too small for both values, so the older one should have been evicted: + assertSame(response2, justTooSmallCache.get(ip2, databasePath2)); + assertNull(justTooSmallCache.get(ip1, databasePath1)); + } + public void testCachesNoResult() { GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); final AtomicInteger count = new AtomicInteger(0); - Function countAndReturnNull = (ip) -> { + Function countAndReturnNull = (ip) -> { count.incrementAndGet(); return null; }; - GeoIpCache.CacheableValue response = cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull); + FakeResponse response = cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull); assertNull(response); assertNull(cache.putIfAbsent("127.0.0.1", "path/to/db", countAndReturnNull)); assertEquals(1, count.get()); @@ -60,8 +89,8 @@ public void testCachesNoResult() { public void testCacheKey() { GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(2); - GeoIpCache.CacheableValue response1 = mock(GeoIpCache.CacheableValue.class); - GeoIpCache.CacheableValue response2 = mock(GeoIpCache.CacheableValue.class); + FakeResponse response1 = new FakeResponse(0); + FakeResponse response2 = new FakeResponse(0); assertSame(response1, cache.putIfAbsent("127.0.0.1", "path/to/db1", ip -> response1)); assertSame(response2, cache.putIfAbsent("127.0.0.1", "path/to/db2", ip -> response2)); @@ -93,7 +122,7 @@ public void testGetCacheStats() { maxCacheSize, () -> testNanoTime.addAndGet(TimeValue.timeValueMillis(1).getNanos()) ); - GeoIpCache.CacheableValue response = mock(GeoIpCache.CacheableValue.class); + FakeResponse response = new FakeResponse(0); String databasePath = "path/to/db1"; String key1 = "127.0.0.1"; String key2 = "127.0.0.2"; From e3b48b1e37d17e831ec71dca8abb4f13724db0be Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 11 Jun 2025 17:41:34 +0100 Subject: [PATCH 06/19] Add a new setting which triggers the byte-based capacity This version of the code does *not* affect the default behaviour. I'm not sure whether that would be considered a breaking change. (We could override the default in serverless, which I am sure we're free to do.) It would obviously be simple to flip the logic around so that we do change the default to, say, 1% of heap (like we do with enrich). --- .../ingest/geoip/GeoIpCache.java | 1 - .../ingest/geoip/IngestGeoIpPlugin.java | 43 +++++++++++++++++-- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index 1aa36e8b39847..61ab05424dfd0 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -45,7 +45,6 @@ static GeoIpCache createGeoIpCacheWithMaxCount(long maxSize) { return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); } - // TODO PETE: Make plugin use this instead of the other factory method when the settings require it static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { if (maxByteSize.getBytes() < 0) { throw new IllegalArgumentException("geoip max cache size in bytes must be 0 or greater"); diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java index c738a33ddf555..6929dcdc29875 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java @@ -24,6 +24,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsFilter; import org.elasticsearch.common.settings.SettingsModule; +import org.elasticsearch.common.unit.ByteSizeValue; +import org.elasticsearch.core.Strings; import org.elasticsearch.features.NodeFeature; import org.elasticsearch.indices.SystemIndexDescriptor; import org.elasticsearch.ingest.EnterpriseGeoIpTask.EnterpriseGeoIpTaskParams; @@ -86,7 +88,17 @@ public class IngestGeoIpPlugin extends Plugin PersistentTaskPlugin, ActionPlugin, ReloadablePlugin { - public static final Setting CACHE_SIZE = Setting.longSetting("ingest.geoip.cache_size", 1000, 0, Setting.Property.NodeScope); + private static final Setting CACHE_SIZE_COUNT = Setting.longSetting( + "ingest.geoip.cache_size", + 1000, + 0, + Setting.Property.NodeScope + ); + private static final Setting CACHE_SIZE_BYTES = Setting.byteSizeSetting( + "ingest.geoip.cache_memory_size", + ByteSizeValue.MINUS_ONE, + Setting.Property.NodeScope + ); private static final int GEOIP_INDEX_MAPPINGS_VERSION = 1; /** * No longer used for determining the age of mappings, but system index descriptor @@ -105,7 +117,8 @@ public class IngestGeoIpPlugin extends Plugin @Override public List> getSettings() { return List.of( - CACHE_SIZE, + CACHE_SIZE_COUNT, + CACHE_SIZE_BYTES, GeoIpDownloaderTaskExecutor.EAGER_DOWNLOAD_SETTING, GeoIpDownloaderTaskExecutor.ENABLED_SETTING, GeoIpDownloader.ENDPOINT_SETTING, @@ -119,8 +132,7 @@ public List> getSettings() { public Map getProcessors(Processor.Parameters parameters) { ingestService.set(parameters.ingestService); - long cacheSize = CACHE_SIZE.get(parameters.env.settings()); - GeoIpCache geoIpCache = GeoIpCache.createGeoIpCacheWithMaxCount(cacheSize); + GeoIpCache geoIpCache = createGeoIpCache(parameters.env.settings()); DatabaseNodeService registry = new DatabaseNodeService( parameters.env, parameters.client, @@ -135,6 +147,29 @@ public Map getProcessors(Processor.Parameters paramet ); } + private static GeoIpCache createGeoIpCache(Settings settings) { + if (settings.hasValue(CACHE_SIZE_BYTES.getKey())) { + if (settings.hasValue(CACHE_SIZE_COUNT.getKey())) { + // Both CACHE_SIZE_COUNT and CACHE_SIZE_BYTES are set, which is an error: + throw new IllegalArgumentException( + Strings.format( + "Both %s and %s are set: please use either %s to set a size based on count or %s to set a size based on bytes of memory", + CACHE_SIZE_COUNT.getKey(), + CACHE_SIZE_BYTES.getKey(), + CACHE_SIZE_COUNT.getKey(), + CACHE_SIZE_BYTES.getKey() + ) + ); + } else { + // Only CACHE_SIZE_BYTES is set, so use that: + return GeoIpCache.createGeoIpCacheWithMaxBytes(CACHE_SIZE_BYTES.get(settings)); + } + } else { + // CACHE_SIZE_BYTES is not set, so use either the explicit or default value of CACHE_SIZE_COUNT: + return GeoIpCache.createGeoIpCacheWithMaxCount(CACHE_SIZE_COUNT.get(settings)); + } + } + @Override public Collection createComponents(PluginServices services) { try { From 30cb5be0e4e9deb1319ee1472008cd1da8a9ae12 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 11 Jun 2025 17:58:47 +0100 Subject: [PATCH 07/19] fix long line --- .../java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java index 6929dcdc29875..30e50a77c33ff 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java @@ -153,7 +153,8 @@ private static GeoIpCache createGeoIpCache(Settings settings) { // Both CACHE_SIZE_COUNT and CACHE_SIZE_BYTES are set, which is an error: throw new IllegalArgumentException( Strings.format( - "Both %s and %s are set: please use either %s to set a size based on count or %s to set a size based on bytes of memory", + "Both %s and %s are set: " + + "please use either %s to set a size based on count or %s to set a size based on bytes of memory", CACHE_SIZE_COUNT.getKey(), CACHE_SIZE_BYTES.getKey(), CACHE_SIZE_COUNT.getKey(), From 0d65209c31329f5f9b8dbf985221e77dd1df16d8 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 12 Jun 2025 17:43:03 +0100 Subject: [PATCH 08/19] Revert changes to `Cacheable[City|Country]Response`, including making them public, just so we can use them in integration tests. Make the tests use their own dummy city and country objects instead. --- .../ingest/geoip/DatabaseNodeServiceIT.java | 3 +- .../ingest/geoip/MaxmindIpDataLookups.java | 95 ++++++++++--------- .../ingest/geoip/ConfigDatabasesTests.java | 5 +- .../ingest/geoip/GeoIpTestUtils.java | 16 ++-- 4 files changed, 62 insertions(+), 57 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index ed55bb91df591..f050dc08b4c25 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -13,7 +13,6 @@ import org.elasticsearch.action.admin.indices.refresh.RefreshRequest; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.common.hash.MessageDigests; -import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCountryResponse; import org.elasticsearch.xcontent.XContentType; import java.io.BufferedInputStream; @@ -94,7 +93,7 @@ private void assertValidDatabase(DatabaseNodeService databaseNodeService, String IpDatabase database = databaseNodeService.getDatabase(databaseFileName); assertNotNull(database); assertThat(database.getDatabaseType(), equalTo(databaseType)); - CacheableCountryResponse countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry); + GeoIpTestUtils.SimpleCountry countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry); assertNotNull(countryResponse); assertThat(countryResponse.countryName(), equalTo("Sweden")); } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 611762e1f6fba..0b29e3f1ae14c 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -229,7 +229,7 @@ protected Map transform(final CacheableAsnResponse response) { } } - public record CacheableCityResponse( + record CacheableCityResponse( Boolean isInEuropeanUnion, String countryIsoCode, String countryName, @@ -246,29 +246,7 @@ public record CacheableCityResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements GeoIpCache.CacheableValue { - - public static CacheableCityResponse from(CityResponse response) { - return new CacheableCityResponse( - MaxmindIpDataLookups.isInEuropeanUnion(response.getCountry()), - response.getCountry().getIsoCode(), - response.getCountry().getName(), - response.getContinent().getCode(), - response.getContinent().getName(), - MaxmindIpDataLookups.regionIsoCode(response.getCountry(), response.getMostSpecificSubdivision()), - response.getMostSpecificSubdivision().getName(), - response.getCity().getName(), - response.getLocation().getTimeZone(), - response.getLocation().getLatitude(), - response.getLocation().getLongitude(), - response.getLocation().getAccuracyRadius(), - response.getPostal().getCode(), - MaxmindIpDataLookups.isInEuropeanUnion(response.getRegisteredCountry()), - response.getRegisteredCountry().getIsoCode(), - response.getRegisteredCountry().getName() - ); - } - } + ) implements GeoIpCache.CacheableValue {} static class City extends AbstractBase> { City(final Set properties) { @@ -277,10 +255,36 @@ static class City extends AbstractBase cacheableRecord(CityResponse response) { + final com.maxmind.geoip2.record.Country country = response.getCountry(); + final Continent continent = response.getContinent(); + final Subdivision subdivision = response.getMostSpecificSubdivision(); + final com.maxmind.geoip2.record.City city = response.getCity(); + final Location location = response.getLocation(); + final Postal postal = response.getPostal(); + final com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); + final Traits traits = response.getTraits(); + return new Result<>( - CacheableCityResponse.from(response), - response.getTraits().getIpAddress(), - response.getTraits().getNetwork().toString() + new CacheableCityResponse( + isInEuropeanUnion(country), + country.getIsoCode(), + country.getName(), + continent.getCode(), + continent.getName(), + regionIsoCode(country, subdivision), + subdivision.getName(), + city.getName(), + location.getTimeZone(), + location.getLatitude(), + location.getLongitude(), + location.getAccuracyRadius(), + postal.getCode(), + isInEuropeanUnion(registeredCountry), + registeredCountry.getIsoCode(), + registeredCountry.getName() + ), + traits.getIpAddress(), + traits.getNetwork().toString() ); } @@ -418,7 +422,7 @@ protected Map transform(final CacheableConnectionTypeResponse re } } - public record CacheableCountryResponse( + record CacheableCountryResponse( Boolean isInEuropeanUnion, String countryIsoCode, String countryName, @@ -427,21 +431,7 @@ public record CacheableCountryResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements GeoIpCache.CacheableValue { - - public static CacheableCountryResponse from(CountryResponse response) { - return new CacheableCountryResponse( - MaxmindIpDataLookups.isInEuropeanUnion(response.getCountry()), - response.getCountry().getIsoCode(), - response.getCountry().getName(), - response.getContinent().getCode(), - response.getContinent().getName(), - MaxmindIpDataLookups.isInEuropeanUnion(response.getRegisteredCountry()), - response.getRegisteredCountry().getIsoCode(), - response.getRegisteredCountry().getName() - ); - } - } + ) implements GeoIpCache.CacheableValue {} static class Country extends AbstractBase> { Country(final Set properties) { @@ -450,10 +440,23 @@ static class Country extends AbstractBase cacheableRecord(CountryResponse response) { + final com.maxmind.geoip2.record.Country country = response.getCountry(); + final Continent continent = response.getContinent(); + final com.maxmind.geoip2.record.Country registeredCountry = response.getRegisteredCountry(); + final Traits traits = response.getTraits(); return new Result<>( - CacheableCountryResponse.from(response), - response.getTraits().getIpAddress(), - response.getTraits().getNetwork().toString() + new CacheableCountryResponse( + isInEuropeanUnion(country), + country.getIsoCode(), + country.getName(), + continent.getCode(), + continent.getName(), + isInEuropeanUnion(registeredCountry), + registeredCountry.getIsoCode(), + registeredCountry.getName() + ), + traits.getIpAddress(), + traits.getNetwork().toString() ); } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java index 1e5555d9cfd8c..38a910c88cc99 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java @@ -11,7 +11,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.TimeValue; -import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCityResponse; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; @@ -125,7 +124,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); + GeoIpTestUtils.SimpleCity cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Tumba")); assertThat(cache.count(), equalTo(1)); } @@ -137,7 +136,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - CacheableCityResponse cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); + GeoIpTestUtils.SimpleCity cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Linköping")); assertThat(cache.count(), equalTo(1)); }); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 72d9e1f59ac5c..bfbf76004b7f0 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -17,8 +17,6 @@ import org.elasticsearch.common.CheckedBiFunction; import org.elasticsearch.common.network.InetAddresses; import org.elasticsearch.core.SuppressForbidden; -import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCityResponse; -import org.elasticsearch.ingest.geoip.MaxmindIpDataLookups.CacheableCountryResponse; import java.io.FileNotFoundException; import java.io.IOException; @@ -69,27 +67,33 @@ static void copyDefaultDatabases(final Path directory, ConfigDatabases configDat } } + public record SimpleCity(String cityName) implements GeoIpCache.CacheableValue {} + /** * A static city-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in * tests. *

* Like this: {@code CacheableCityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} */ - public static CacheableCityResponse getCity(Reader reader, String ip) throws IOException { + public static SimpleCity getCity(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CityResponse.class); CityResponse data = record.getData(); - return data == null ? null : CacheableCityResponse.from(new CityResponse(data, ip, record.getNetwork(), List.of("en"))); + return data == null ? null : new SimpleCity(new CityResponse(data, ip, record.getNetwork(), List.of("en")).getCity().getName()); } + public record SimpleCountry(String countryName) implements GeoIpCache.CacheableValue {} + /** * A static country-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in * tests. *

* Like this: {@code CacheableCountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} */ - public static CacheableCountryResponse getCountry(Reader reader, String ip) throws IOException { + public static SimpleCountry getCountry(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CountryResponse.class); CountryResponse data = record.getData(); - return data == null ? null : CacheableCountryResponse.from(new CountryResponse(data, ip, record.getNetwork(), List.of("en"))); + return data == null + ? null + : new SimpleCountry(new CountryResponse(data, ip, record.getNetwork(), List.of("en")).getCountry().getName()); } } From 91fa87b948e094d38c8c36038d11b7307894b220 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 12 Jun 2025 17:46:08 +0100 Subject: [PATCH 09/19] correct comment --- .../java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index bfbf76004b7f0..306fa5f88f306 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -73,7 +73,7 @@ public record SimpleCity(String cityName) implements GeoIpCache.CacheableValue { * A static city-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in * tests. *

- * Like this: {@code CacheableCityResponse city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} + * Like this: {@code SimpleCity city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} */ public static SimpleCity getCity(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CityResponse.class); @@ -87,7 +87,7 @@ public record SimpleCountry(String countryName) implements GeoIpCache.CacheableV * A static country-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in * tests. *

- * Like this: {@code CacheableCountryResponse country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} + * Like this: {@code SimpleCountry country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} */ public static SimpleCountry getCountry(Reader reader, String ip) throws IOException { DatabaseRecord record = reader.getRecord(InetAddresses.forString(ip), CountryResponse.class); From 7759a1e24c78a826727fe5e37aaf5abf17fd953b Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 18 Jun 2025 13:05:42 +0100 Subject: [PATCH 10/19] minor tweaks for readability, test coverage --- .../ingest/geoip/IngestGeoIpPlugin.java | 2 ++ .../ingest/geoip/GeoIpCacheTests.java | 16 +++++++++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java index 30e50a77c33ff..e785a1b83ac1f 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java @@ -88,12 +88,14 @@ public class IngestGeoIpPlugin extends Plugin PersistentTaskPlugin, ActionPlugin, ReloadablePlugin { + private static final Setting CACHE_SIZE_COUNT = Setting.longSetting( "ingest.geoip.cache_size", 1000, 0, Setting.Property.NodeScope ); + private static final Setting CACHE_SIZE_BYTES = Setting.byteSizeSetting( "ingest.geoip.cache_memory_size", ByteSizeValue.MINUS_ONE, diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index 3f4644b83310d..563ce3cc0993c 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -54,10 +54,8 @@ public void testCachesAndEvictsResults_maxBytes() { String ip2 = "127.0.0.2"; String databasePath2 = "path/to/db"; FakeResponse response2 = new FakeResponse(222); - long totalSize = GeoIpCache.keySizeInBytes(ip1, databasePath1) + response1.sizeInBytes() + GeoIpCache.keySizeInBytes( - ip2, - databasePath2 - ) + response2.sizeInBytes(); + long totalSize = GeoIpCache.keySizeInBytes(ip1, databasePath1) + GeoIpCache.keySizeInBytes(ip2, databasePath2) + response1 + .sizeInBytes() + response2.sizeInBytes(); GeoIpCache justBigEnoughCache = GeoIpCache.createGeoIpCacheWithMaxBytes(ByteSizeValue.ofBytes(totalSize)); justBigEnoughCache.putIfAbsent(projectId, ip1, databasePath1, ip -> response1); @@ -129,11 +127,19 @@ public void testThrowsFunctionsException() { assertEquals("bad", ex.getMessage()); } - public void testInvalidInit() { + public void testInvalidInit_maxCount() { IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> GeoIpCache.createGeoIpCacheWithMaxCount(-1)); assertEquals("geoip max cache size must be 0 or greater", ex.getMessage()); } + public void testInvalidInit_maxBytes() { + IllegalArgumentException ex = expectThrows( + IllegalArgumentException.class, + () -> GeoIpCache.createGeoIpCacheWithMaxBytes(ByteSizeValue.MINUS_ONE) + ); + assertEquals("geoip max cache size in bytes must be 0 or greater", ex.getMessage()); + } + public void testGetCacheStats() { final long maxCacheSize = 2; final AtomicLong testNanoTime = new AtomicLong(0); From 3fe30eca30e00708bb9768b295aae1afe5c5e554 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 18 Jun 2025 13:10:58 +0100 Subject: [PATCH 11/19] Add size of project ID to key size --- .../java/org/elasticsearch/ingest/geoip/GeoIpCache.java | 7 +++---- .../org/elasticsearch/ingest/geoip/GeoIpCacheTests.java | 4 ++-- .../java/org/elasticsearch/cluster/metadata/ProjectId.java | 6 ++++++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index 89e55b05e6419..f66f3de4cb577 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -173,13 +173,12 @@ private record CacheKey(ProjectId projectId, String ip, String databasePath) { private static final long BASE_BYTES = RamUsageEstimator.shallowSizeOfInstance(CacheKey.class); private long sizeInBytes() { - return keySizeInBytes(ip, databasePath); + return keySizeInBytes(projectId, ip, databasePath); } } // visible for testing - static long keySizeInBytes(String ip, String databasePath) { - // TODO(pete): Add ProjectID size - return CacheKey.BASE_BYTES + RamUsageEstimator.sizeOf(ip) + RamUsageEstimator.sizeOf(databasePath); + static long keySizeInBytes(ProjectId projectId, String ip, String databasePath) { + return CacheKey.BASE_BYTES + projectId.sizeInBytes() + RamUsageEstimator.sizeOf(ip) + RamUsageEstimator.sizeOf(databasePath); } } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index 563ce3cc0993c..219af60033e08 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -54,8 +54,8 @@ public void testCachesAndEvictsResults_maxBytes() { String ip2 = "127.0.0.2"; String databasePath2 = "path/to/db"; FakeResponse response2 = new FakeResponse(222); - long totalSize = GeoIpCache.keySizeInBytes(ip1, databasePath1) + GeoIpCache.keySizeInBytes(ip2, databasePath2) + response1 - .sizeInBytes() + response2.sizeInBytes(); + long totalSize = GeoIpCache.keySizeInBytes(projectId, ip1, databasePath1) + GeoIpCache.keySizeInBytes(projectId, ip2, databasePath2) + + response1.sizeInBytes() + response2.sizeInBytes(); GeoIpCache justBigEnoughCache = GeoIpCache.createGeoIpCacheWithMaxBytes(ByteSizeValue.ofBytes(totalSize)); justBigEnoughCache.putIfAbsent(projectId, ip1, databasePath1, ip -> response1); diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/ProjectId.java b/server/src/main/java/org/elasticsearch/cluster/metadata/ProjectId.java index 94fa0164b5fbe..bdc9a32092cab 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/ProjectId.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/ProjectId.java @@ -9,6 +9,7 @@ package org.elasticsearch.cluster.metadata; +import org.apache.lucene.util.RamUsageEstimator; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -27,6 +28,7 @@ public class ProjectId implements Writeable, ToXContent { public static final ProjectId DEFAULT = new ProjectId(DEFAULT_STRING); public static final Reader READER = ProjectId::readFrom; private static final int MAX_LENGTH = 128; + private static final long BASE_BYTES_SIZE = RamUsageEstimator.shallowSizeOfInstance(ProjectId.class); private final String id; @@ -113,4 +115,8 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hashCode(id); } + + public long sizeInBytes() { + return BASE_BYTES_SIZE + RamUsageEstimator.sizeOf(id); + } } From 88869f851b8c421bf1d0e84d3050d76700f3c2bf Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Wed, 18 Jun 2025 12:20:36 +0000 Subject: [PATCH 12/19] [CI] Auto commit changes from spotless --- .../elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index e8e844bdb86e3..c055e471cb31d 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -108,7 +108,10 @@ int current() { @Override @Nullable @FixForMultiProject // do not use ProjectId.DEFAULT - public RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { + public RESPONSE getResponse( + String ipAddress, + CheckedBiFunction responseProvider + ) { return cache.putIfAbsent(ProjectId.DEFAULT, ipAddress, cachedDatabasePathToString, ip -> { try { return responseProvider.apply(get(), ipAddress); From 0f1738d675981d93079eb71032da0de8139d293a Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 19 Jun 2025 15:33:05 +0100 Subject: [PATCH 13/19] Move base and rename response interface --- .../geoip/DatabaseReaderLazyLoader.java | 2 +- .../ingest/geoip/GeoIpCache.java | 27 ++++++++----------- .../ingest/geoip/IpDataLookup.java | 2 +- .../ingest/geoip/IpDatabase.java | 10 ++++++- .../ingest/geoip/IpinfoIpDataLookups.java | 10 +++---- .../ingest/geoip/MaxmindIpDataLookups.java | 18 ++++++------- .../ingest/geoip/GeoIpCacheTests.java | 2 +- .../ingest/geoip/GeoIpTestUtils.java | 4 +-- 8 files changed, 39 insertions(+), 36 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index c055e471cb31d..d124b0e594d7e 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -108,7 +108,7 @@ int current() { @Override @Nullable @FixForMultiProject // do not use ProjectId.DEFAULT - public RESPONSE getResponse( + public RESPONSE getResponse( String ipAddress, CheckedBiFunction responseProvider ) { diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index f66f3de4cb577..b1da74dece926 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -31,19 +31,11 @@ */ public final class GeoIpCache { - public interface CacheableValue { - - // TODO PETE: Remove this default implementation and implement in all implementing classes instead - default long sizeInBytes() { - return 0; - } - } - static GeoIpCache createGeoIpCacheWithMaxCount(long maxSize) { if (maxSize < 0) { throw new IllegalArgumentException("geoip max cache size must be 0 or greater"); } - return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); + return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); } static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { @@ -52,7 +44,7 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { } return new GeoIpCache( System::nanoTime, - CacheBuilder.builder() + CacheBuilder.builder() .setMaximumWeight(maxByteSize.getBytes()) .weigher((key, value) -> key.sizeInBytes() + value.sizeInBytes()) .build() @@ -61,7 +53,10 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { // package private for testing static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize, LongSupplier relativeNanoTimeProvider) { - return new GeoIpCache(relativeNanoTimeProvider, CacheBuilder.builder().setMaximumWeight(maxSize).build()); + return new GeoIpCache( + relativeNanoTimeProvider, + CacheBuilder.builder().setMaximumWeight(maxSize).build() + ); } /** @@ -70,7 +65,7 @@ static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize * something not being in the cache because the data doesn't exist in the database. */ // visible for testing - static final CacheableValue NO_RESULT = new CacheableValue() { + static final IpDatabase.Response NO_RESULT = new IpDatabase.Response() { @Override public String toString() { return "NO_RESULT"; @@ -78,17 +73,17 @@ public String toString() { }; private final LongSupplier relativeNanoTimeProvider; - private final Cache cache; + private final Cache cache; private final AtomicLong hitsTimeInNanos = new AtomicLong(0); private final AtomicLong missesTimeInNanos = new AtomicLong(0); - private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { + private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { this.relativeNanoTimeProvider = relativeNanoTimeProvider; this.cache = cache; } @SuppressWarnings("unchecked") - RESPONSE putIfAbsent( + RESPONSE putIfAbsent( ProjectId projectId, String ip, String databasePath, @@ -98,7 +93,7 @@ RESPONSE putIfAbsent( CacheKey cacheKey = new CacheKey(projectId, ip, databasePath); long cacheStart = relativeNanoTimeProvider.getAsLong(); // intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition. - CacheableValue response = cache.get(cacheKey); + IpDatabase.Response response = cache.get(cacheKey); long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart; // populate the cache for this key, if necessary diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java index 9319979542a73..1b9457eb366b0 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java @@ -34,5 +34,5 @@ interface IpDataLookup { * as a network for which the record applies. Having a helper record prevents each individual response record from needing to * track these bits of information. */ - record Result(T result, String ip, String network) implements GeoIpCache.CacheableValue {} + record Result(T result, String ip, String network) implements IpDatabase.Response {} } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java index c3faa10c20010..645433a5b75c0 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java @@ -35,7 +35,7 @@ public interface IpDatabase extends AutoCloseable { * @return a possibly-null response */ @Nullable - RESPONSE getResponse( + RESPONSE getResponse( String ipAddress, CheckedBiFunction responseProvider ); @@ -48,4 +48,12 @@ RESPONSE getResponse( */ @Override void close() throws IOException; + + interface Response { + + // TODO PETE: Remove this default implementation and implement in all implementing classes instead + default long sizeInBytes() { + return 0; + } + } } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index ce6da6806283f..52fd345ce28b7 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -191,7 +191,7 @@ public record AsnResult( String domain, String name, @Nullable String type // not present in the free asn database - ) implements GeoIpCache.CacheableValue { + ) implements IpDatabase.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public AsnResult( @@ -210,14 +210,14 @@ public record CountryResult( @MaxMindDbParameter(name = "continent_name") String continentName, @MaxMindDbParameter(name = "country") String country, @MaxMindDbParameter(name = "country_name") String countryName - ) implements GeoIpCache.CacheableValue { + ) implements IpDatabase.Response { @MaxMindDbConstructor public CountryResult {} } public record GeolocationResult(String city, String country, Double lat, Double lng, String postalCode, String region, String timezone) implements - GeoIpCache.CacheableValue { + IpDatabase.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public GeolocationResult( @@ -237,7 +237,7 @@ public GeolocationResult( public record PrivacyDetectionResult(Boolean hosting, Boolean proxy, Boolean relay, String service, Boolean tor, Boolean vpn) implements - GeoIpCache.CacheableValue { + IpDatabase.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public PrivacyDetectionResult( @@ -462,7 +462,7 @@ protected Map transform(final Result res * * @param the record type that will be wrapped and returned */ - private abstract static class AbstractBase implements IpDataLookup { + private abstract static class AbstractBase implements IpDataLookup { protected final Set properties; protected final Class clazz; diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 0b29e3f1ae14c..812c4892f803e 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -109,7 +109,7 @@ static Function, IpDataLookup> getMaxmindLookup(final Dat }; } - static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements GeoIpCache.CacheableValue { + static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements IpDatabase.Response { CacheableAnonymousIpResponse(AnonymousIpResponse response) { super( @@ -176,7 +176,7 @@ protected Map transform(final CacheableAnonymousIpResponse respo } } - static class CacheableAsnResponse extends AsnResponse implements GeoIpCache.CacheableValue { + static class CacheableAsnResponse extends AsnResponse implements IpDatabase.Response { CacheableAsnResponse(AsnResponse response) { super( @@ -246,7 +246,7 @@ record CacheableCityResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements GeoIpCache.CacheableValue {} + ) implements IpDatabase.Response {} static class City extends AbstractBase> { City(final Set properties) { @@ -382,7 +382,7 @@ protected Map transform(final Result resu } } - static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements GeoIpCache.CacheableValue { + static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements IpDatabase.Response { CacheableConnectionTypeResponse(ConnectionTypeResponse response) { super(response.getConnectionType(), response.getIpAddress(), response.getNetwork()); @@ -431,7 +431,7 @@ record CacheableCountryResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements GeoIpCache.CacheableValue {} + ) implements IpDatabase.Response {} static class Country extends AbstractBase> { Country(final Set properties) { @@ -514,7 +514,7 @@ protected Map transform(final Result r } } - static class CacheableDomainResponse extends DomainResponse implements GeoIpCache.CacheableValue { + static class CacheableDomainResponse extends DomainResponse implements IpDatabase.Response { CacheableDomainResponse(DomainResponse response) { super(response.getDomain(), response.getIpAddress(), response.getNetwork()); @@ -589,7 +589,7 @@ record CacheableEnterpriseResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements GeoIpCache.CacheableValue {} + ) implements IpDatabase.Response {} static class Enterprise extends AbstractBase> { Enterprise(final Set properties) { @@ -826,7 +826,7 @@ protected Map transform(final Result { * * @param the intermediate type of {@link AbstractResponse} */ - private abstract static class AbstractBase + private abstract static class AbstractBase implements IpDataLookup { diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index fa902de20dc98..7b3ff0e2865b5 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -25,7 +25,7 @@ public class GeoIpCacheTests extends ESTestCase { - private record FakeResponse(long sizeInBytes) implements GeoIpCache.CacheableValue {} + private record FakeResponse(long sizeInBytes) implements IpDatabase.Response {} public void testCachesAndEvictsResults_maxCount() { GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 306fa5f88f306..288703972ab1d 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -67,7 +67,7 @@ static void copyDefaultDatabases(final Path directory, ConfigDatabases configDat } } - public record SimpleCity(String cityName) implements GeoIpCache.CacheableValue {} + public record SimpleCity(String cityName) implements IpDatabase.Response {} /** * A static city-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in @@ -81,7 +81,7 @@ public static SimpleCity getCity(Reader reader, String ip) throws IOException { return data == null ? null : new SimpleCity(new CityResponse(data, ip, record.getNetwork(), List.of("en")).getCity().getName()); } - public record SimpleCountry(String countryName) implements GeoIpCache.CacheableValue {} + public record SimpleCountry(String countryName) implements IpDatabase.Response {} /** * A static country-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in From 48a4d556c1037b2188484df721a486b3bc41cc14 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 19 Jun 2025 16:04:53 +0100 Subject: [PATCH 14/19] Do a horrible thing as an experimental workaround for the logstash issue --- .../ingest/geoip/DatabaseNodeServiceIT.java | 2 +- ...gDatabasesWhilePerformingGeoLookupsIT.java | 4 +-- .../geoip/DatabaseReaderLazyLoader.java | 3 +- .../ingest/geoip/IpDatabase.java | 32 +++++++++++++++++-- .../ingest/geoip/IpinfoIpDataLookups.java | 2 +- .../ingest/geoip/MaxmindIpDataLookups.java | 2 +- .../ingest/geoip/ConfigDatabasesTests.java | 4 +-- .../ingest/geoip/GeoIpTestUtils.java | 4 +-- 8 files changed, 40 insertions(+), 13 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index f050dc08b4c25..300147eea43e1 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -93,7 +93,7 @@ private void assertValidDatabase(DatabaseNodeService databaseNodeService, String IpDatabase database = databaseNodeService.getDatabase(databaseFileName); assertNotNull(database); assertThat(database.getDatabaseType(), equalTo(databaseType)); - GeoIpTestUtils.SimpleCountry countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry); + GeoIpTestUtils.SimpleCountry countryResponse = database.getTypedResponse("89.160.20.128", GeoIpTestUtils::getCountry); assertNotNull(countryResponse); assertThat(countryResponse.countryName(), equalTo("Sweden")); } diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java index 8881f675a8d22..cc42debaaf829 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java @@ -213,10 +213,10 @@ private static DatabaseNodeService createRegistry(Path geoIpConfigDir, Path geoI private static void lazyLoadReaders(DatabaseNodeService databaseNodeService) throws IOException { if (databaseNodeService.get("GeoLite2-City.mmdb") != null) { databaseNodeService.get("GeoLite2-City.mmdb").getDatabaseType(); - databaseNodeService.get("GeoLite2-City.mmdb").getResponse("2.125.160.216", GeoIpTestUtils::getCity); + databaseNodeService.get("GeoLite2-City.mmdb").getTypedResponse("2.125.160.216", GeoIpTestUtils::getCity); } databaseNodeService.get("GeoLite2-City-Test.mmdb").getDatabaseType(); - databaseNodeService.get("GeoLite2-City-Test.mmdb").getResponse("2.125.160.216", GeoIpTestUtils::getCity); + databaseNodeService.get("GeoLite2-City-Test.mmdb").getTypedResponse("2.125.160.216", GeoIpTestUtils::getCity); } } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index d124b0e594d7e..9337a9001e01c 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -105,10 +105,9 @@ int current() { return currentUsages.get(); } - @Override @Nullable @FixForMultiProject // do not use ProjectId.DEFAULT - public RESPONSE getResponse( + public RESPONSE getTypedResponse( String ipAddress, CheckedBiFunction responseProvider ) { diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java index 645433a5b75c0..05983abe33e08 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java @@ -35,10 +35,38 @@ public interface IpDatabase extends AutoCloseable { * @return a possibly-null response */ @Nullable - RESPONSE getResponse( + @Deprecated // use getTypedResponse instead + default RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { + // TODO(pete): Decide what to do here + // TODO(pete): This is one option, which makes this method work so long as it is invoked with a type which satisfies the constraint, + // but fails nastily if it doesn't. + // @SuppressWarnings("unchecked") // TODO(pete): Put some words of wisdom here + // RESPONSE typedResponse = (RESPONSE) getTypedResponse( + // ipAddress, + // (CheckedBiFunction) responseProvider + // ); + // return typedResponse; + // TODO(pete): This is the other option, which fails fast. Which is okay as nothing calls this method. + // TODO(pete): If we do this, add a message to the exception. + throw new UnsupportedOperationException(); + } + + // TODO(pete): If we end up doing this, pick a better name for getTypedResponse() and document why we do this weird thing. + + /** + * Returns a response from this database's reader for the given IP address. + * + * @param ipAddress the address to lookup + * @param responseProvider a method for extracting a response from a {@link Reader}, usually this will be a method reference + * @return a possibly-null response + */ + @Nullable + default RESPONSE getTypedResponse( String ipAddress, CheckedBiFunction responseProvider - ); + ) { + return getResponse(ipAddress, responseProvider); + } /** * Releases the current database object. Called after processing a single document. Databases should be closed or returned to a diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index 52fd345ce28b7..0840d1bb0751d 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -479,7 +479,7 @@ public Set getProperties() { @Override public final Map getData(final IpDatabase ipDatabase, final String ipAddress) { - final Result response = ipDatabase.getResponse(ipAddress, this::lookup); + final Result response = ipDatabase.getTypedResponse(ipAddress, this::lookup); return (response == null || response.result() == null) ? Map.of() : transform(response); } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 812c4892f803e..b76d864594fed 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -938,7 +938,7 @@ public Set getProperties() { @Override public final Map getData(final IpDatabase ipDatabase, final String ipAddress) { - final RECORD response = ipDatabase.getResponse(ipAddress, this::lookup); + final RECORD response = ipDatabase.getTypedResponse(ipAddress, this::lookup); return (response == null) ? Map.of() : transform(response); } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java index 38a910c88cc99..37f4d72927053 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java @@ -124,7 +124,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - GeoIpTestUtils.SimpleCity cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); + GeoIpTestUtils.SimpleCity cityResponse = loader.getTypedResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Tumba")); assertThat(cache.count(), equalTo(1)); } @@ -136,7 +136,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - GeoIpTestUtils.SimpleCity cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); + GeoIpTestUtils.SimpleCity cityResponse = loader.getTypedResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Linköping")); assertThat(cache.count(), equalTo(1)); }); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 288703972ab1d..93aa37ad8d886 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -70,7 +70,7 @@ static void copyDefaultDatabases(final Path directory, ConfigDatabases configDat public record SimpleCity(String cityName) implements IpDatabase.Response {} /** - * A static city-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in + * A static city-specific responseProvider for use with {@link IpDatabase#getTypedResponse(String, CheckedBiFunction)} in * tests. *

* Like this: {@code SimpleCity city = loader.getResponse("some.ip.address", GeoIpTestUtils::getCity);} @@ -84,7 +84,7 @@ public static SimpleCity getCity(Reader reader, String ip) throws IOException { public record SimpleCountry(String countryName) implements IpDatabase.Response {} /** - * A static country-specific responseProvider for use with {@link IpDatabase#getResponse(String, CheckedBiFunction)} in + * A static country-specific responseProvider for use with {@link IpDatabase#getTypedResponse(String, CheckedBiFunction)} in * tests. *

* Like this: {@code SimpleCountry country = loader.getResponse("some.ip.address", GeoIpTestUtils::getCountry);} From 45741c14b8dfa48ece20a1c8e3c6860ce8156842 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 19 Jun 2025 18:52:35 +0100 Subject: [PATCH 15/19] Tweaking the horrible thing --- .../geoip/DatabaseReaderLazyLoader.java | 8 +++++ .../ingest/geoip/IpDatabase.java | 29 ++++++++++--------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index 9337a9001e01c..d112da074eeff 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -105,6 +105,14 @@ int current() { return currentUsages.get(); } + @Override + @Nullable + @Deprecated // use getTypedResponse instead + public RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { + throw new UnsupportedOperationException(); // TODO(pete): Write some words of wisdom in an exception message + } + + @Override @Nullable @FixForMultiProject // do not use ProjectId.DEFAULT public RESPONSE getTypedResponse( diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java index 05983abe33e08..709f47c2c083c 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java @@ -27,6 +27,19 @@ public interface IpDatabase extends AutoCloseable { */ String getDatabaseType() throws IOException; + /* + * TODO(pete): Add an explanation of all this to the class- and method-level javadocs. + * + * This class has two related methods, getResponse and a method getTypedResponse. These are identical except that getTypedResponse's + * generic type is constrained to extend the Response interface. The getResponse method is abstract. The default implementation of + * getTypedResponse delegates to getResponse and that is the only place in the codebase that getResponse is called: everywhere else + * calls getTypedResponse (as of ES 9.1.0). Implementing classes therefore have a choice: they can implement getResponse; or they can + * override the implementation of getTypedResponse, and make getResponse throw since it will now never be called. + * + * (The getResponse method only exists because there are implementations of this interface outside of this codebase which haven't yet + * been converted to implement getTypedResponse instead.) + */ + /** * Returns a response from this database's reader for the given IP address. * @@ -36,20 +49,8 @@ public interface IpDatabase extends AutoCloseable { */ @Nullable @Deprecated // use getTypedResponse instead - default RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { - // TODO(pete): Decide what to do here - // TODO(pete): This is one option, which makes this method work so long as it is invoked with a type which satisfies the constraint, - // but fails nastily if it doesn't. - // @SuppressWarnings("unchecked") // TODO(pete): Put some words of wisdom here - // RESPONSE typedResponse = (RESPONSE) getTypedResponse( - // ipAddress, - // (CheckedBiFunction) responseProvider - // ); - // return typedResponse; - // TODO(pete): This is the other option, which fails fast. Which is okay as nothing calls this method. - // TODO(pete): If we do this, add a message to the exception. - throw new UnsupportedOperationException(); - } + // TODO(pete): If we're allowed to remove this in v10, annotate it to remind us to do that. + RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider); // TODO(pete): If we end up doing this, pick a better name for getTypedResponse() and document why we do this weird thing. From eb900236ba5f80d34bc91f4483e102bb38c3eef5 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 10 Jul 2025 19:46:12 +0100 Subject: [PATCH 16/19] Do a horrible ugly thing. This throws away some type safety, and requires us to make sure that we only ever call getResponse() with types which satisfy the constraint. It allows us to not touch IpDatabase. --- .../ingest/geoip/DatabaseNodeServiceIT.java | 2 +- ...gDatabasesWhilePerformingGeoLookupsIT.java | 6 ++- .../geoip/DatabaseReaderLazyLoader.java | 24 +++++------ .../ingest/geoip/GeoIpCache.java | 16 ++++---- .../ingest/geoip/IpDataLookup.java | 22 +++++++++- .../ingest/geoip/IpDatabase.java | 41 +------------------ .../ingest/geoip/IpinfoIpDataLookups.java | 12 +++--- .../ingest/geoip/MaxmindIpDataLookups.java | 20 ++++----- .../ingest/geoip/ConfigDatabasesTests.java | 4 +- .../ingest/geoip/GeoIpCacheTests.java | 2 +- .../ingest/geoip/GeoIpTestUtils.java | 4 +- 11 files changed, 68 insertions(+), 85 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index 6efb5917a1ab4..811da47b9eb59 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -99,7 +99,7 @@ private void assertValidDatabase(DatabaseNodeService databaseNodeService, String IpDatabase database = databaseNodeService.getDatabase(projectId, databaseFileName); assertNotNull(database); assertThat(database.getDatabaseType(), equalTo(databaseType)); - GeoIpTestUtils.SimpleCountry countryResponse = database.getTypedResponse("89.160.20.128", GeoIpTestUtils::getCountry); + GeoIpTestUtils.SimpleCountry countryResponse = database.getResponse("89.160.20.128", GeoIpTestUtils::getCountry); assertNotNull(countryResponse); assertThat(countryResponse.countryName(), equalTo("Sweden")); } diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java index 57d7fc970883f..77d0349910795 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/ReloadingDatabasesWhilePerformingGeoLookupsIT.java @@ -242,10 +242,12 @@ private static DatabaseNodeService createRegistry( private static void lazyLoadReaders(ProjectId projectId, DatabaseNodeService databaseNodeService) throws IOException { if (databaseNodeService.get(projectId, "GeoLite2-City.mmdb") != null) { databaseNodeService.get(projectId, "GeoLite2-City.mmdb").getDatabaseType(); - databaseNodeService.get(projectId, "GeoLite2-City.mmdb").getTypedResponse("2.125.160.216", GeoIpTestUtils::getCity); + IpDatabase ipDatabase = databaseNodeService.get(projectId, "GeoLite2-City.mmdb"); + ipDatabase.getResponse("2.125.160.216", GeoIpTestUtils::getCity); } databaseNodeService.get(projectId, "GeoLite2-City-Test.mmdb").getDatabaseType(); - databaseNodeService.get(projectId, "GeoLite2-City-Test.mmdb").getTypedResponse("2.125.160.216", GeoIpTestUtils::getCity); + IpDatabase ipDatabase = databaseNodeService.get(projectId, "GeoLite2-City-Test.mmdb"); + ipDatabase.getResponse("2.125.160.216", GeoIpTestUtils::getCity); } } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index 38cdf408bcf06..9e324f4ffc217 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -30,6 +30,7 @@ import java.nio.file.Path; import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; /** * Facilitates lazy loading of the database reader, so that when the geoip plugin is installed, but not used, @@ -115,24 +116,23 @@ int current() { @Override @Nullable - @Deprecated // use getTypedResponse instead public RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { - throw new UnsupportedOperationException(); // TODO(pete): Write some words of wisdom in an exception message - } - - @Override - @Nullable - public RESPONSE getTypedResponse( - String ipAddress, - CheckedBiFunction responseProvider - ) { - return cache.putIfAbsent(projectId, ipAddress, cachedDatabasePathToString, ip -> { + Function retrieveFunction = ip -> { try { return responseProvider.apply(get(), ipAddress); } catch (Exception e) { throw ExceptionsHelper.convertToRuntime(e); } - }); + }; + @SuppressWarnings("unchecked") // This is not generally safe! However, it is okay since we know that we will only call getResponse + // with RESPONSE types which extend IpDataLookup.Response. TODO(pete): Explain why we're doing this! + Function castRetrieveFunction = (Function< + String, + ? extends IpDataLookup.Response>) retrieveFunction; + @SuppressWarnings("unchecked") // This one *is* safe, because we know that the retrieve function really does return RESPONSE (it was + // typed as such before we cast it). + RESPONSE response = (RESPONSE) cache.putIfAbsent(projectId, ipAddress, cachedDatabasePathToString, castRetrieveFunction); + return response; } Reader get() throws IOException { diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index b1da74dece926..c8d7171697e5f 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -35,7 +35,7 @@ static GeoIpCache createGeoIpCacheWithMaxCount(long maxSize) { if (maxSize < 0) { throw new IllegalArgumentException("geoip max cache size must be 0 or greater"); } - return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); + return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); } static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { @@ -44,7 +44,7 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { } return new GeoIpCache( System::nanoTime, - CacheBuilder.builder() + CacheBuilder.builder() .setMaximumWeight(maxByteSize.getBytes()) .weigher((key, value) -> key.sizeInBytes() + value.sizeInBytes()) .build() @@ -55,7 +55,7 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize, LongSupplier relativeNanoTimeProvider) { return new GeoIpCache( relativeNanoTimeProvider, - CacheBuilder.builder().setMaximumWeight(maxSize).build() + CacheBuilder.builder().setMaximumWeight(maxSize).build() ); } @@ -65,7 +65,7 @@ static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize * something not being in the cache because the data doesn't exist in the database. */ // visible for testing - static final IpDatabase.Response NO_RESULT = new IpDatabase.Response() { + static final IpDataLookup.Response NO_RESULT = new IpDataLookup.Response() { @Override public String toString() { return "NO_RESULT"; @@ -73,17 +73,17 @@ public String toString() { }; private final LongSupplier relativeNanoTimeProvider; - private final Cache cache; + private final Cache cache; private final AtomicLong hitsTimeInNanos = new AtomicLong(0); private final AtomicLong missesTimeInNanos = new AtomicLong(0); - private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { + private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { this.relativeNanoTimeProvider = relativeNanoTimeProvider; this.cache = cache; } @SuppressWarnings("unchecked") - RESPONSE putIfAbsent( + RESPONSE putIfAbsent( ProjectId projectId, String ip, String databasePath, @@ -93,7 +93,7 @@ RESPONSE putIfAbsent( CacheKey cacheKey = new CacheKey(projectId, ip, databasePath); long cacheStart = relativeNanoTimeProvider.getAsLong(); // intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition. - IpDatabase.Response response = cache.get(cacheKey); + IpDataLookup.Response response = cache.get(cacheKey); long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart; // populate the cache for this key, if necessary diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java index 1b9457eb366b0..108fab921e31b 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java @@ -9,6 +9,10 @@ package org.elasticsearch.ingest.geoip; +import com.maxmind.db.Reader; + +import org.elasticsearch.common.CheckedBiFunction; + import java.io.IOException; import java.util.Map; import java.util.Set; @@ -24,15 +28,31 @@ interface IpDataLookup { */ Map getData(IpDatabase ipDatabase, String ip) throws IOException; + default RECORD databaseLookup( + IpDatabase ipDatabase, + String ipAddress, + CheckedBiFunction responseProvider + ) { + return ipDatabase.getResponse(ipAddress, responseProvider); + } + /** * @return the set of properties this lookup will provide */ Set getProperties(); + interface Response { + + // TODO PETE: Remove this default implementation and implement in all implementing classes instead + default long sizeInBytes() { + return 0; + } + } + /** * A helper record that holds other records. Every ip data lookup will have an associated ip address that was looked up, as well * as a network for which the record applies. Having a helper record prevents each individual response record from needing to * track these bits of information. */ - record Result(T result, String ip, String network) implements IpDatabase.Response {} + record Result(T result, String ip, String network) implements Response {} } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java index 709f47c2c083c..db1ffc1c682b8 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDatabase.java @@ -27,48 +27,17 @@ public interface IpDatabase extends AutoCloseable { */ String getDatabaseType() throws IOException; - /* - * TODO(pete): Add an explanation of all this to the class- and method-level javadocs. - * - * This class has two related methods, getResponse and a method getTypedResponse. These are identical except that getTypedResponse's - * generic type is constrained to extend the Response interface. The getResponse method is abstract. The default implementation of - * getTypedResponse delegates to getResponse and that is the only place in the codebase that getResponse is called: everywhere else - * calls getTypedResponse (as of ES 9.1.0). Implementing classes therefore have a choice: they can implement getResponse; or they can - * override the implementation of getTypedResponse, and make getResponse throw since it will now never be called. - * - * (The getResponse method only exists because there are implementations of this interface outside of this codebase which haven't yet - * been converted to implement getTypedResponse instead.) - */ - /** * Returns a response from this database's reader for the given IP address. * * @param ipAddress the address to lookup * @param responseProvider a method for extracting a response from a {@link Reader}, usually this will be a method reference * @return a possibly-null response + * @param the type of response that will be returned */ @Nullable - @Deprecated // use getTypedResponse instead - // TODO(pete): If we're allowed to remove this in v10, annotate it to remind us to do that. RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider); - // TODO(pete): If we end up doing this, pick a better name for getTypedResponse() and document why we do this weird thing. - - /** - * Returns a response from this database's reader for the given IP address. - * - * @param ipAddress the address to lookup - * @param responseProvider a method for extracting a response from a {@link Reader}, usually this will be a method reference - * @return a possibly-null response - */ - @Nullable - default RESPONSE getTypedResponse( - String ipAddress, - CheckedBiFunction responseProvider - ) { - return getResponse(ipAddress, responseProvider); - } - /** * Releases the current database object. Called after processing a single document. Databases should be closed or returned to a * resource pool. No further interactions should be expected. @@ -77,12 +46,4 @@ default RESPONSE getTypedResponse( */ @Override void close() throws IOException; - - interface Response { - - // TODO PETE: Remove this default implementation and implement in all implementing classes instead - default long sizeInBytes() { - return 0; - } - } } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index 0840d1bb0751d..a859e533aa7a8 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -191,7 +191,7 @@ public record AsnResult( String domain, String name, @Nullable String type // not present in the free asn database - ) implements IpDatabase.Response { + ) implements IpDataLookup.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public AsnResult( @@ -210,14 +210,14 @@ public record CountryResult( @MaxMindDbParameter(name = "continent_name") String continentName, @MaxMindDbParameter(name = "country") String country, @MaxMindDbParameter(name = "country_name") String countryName - ) implements IpDatabase.Response { + ) implements IpDataLookup.Response { @MaxMindDbConstructor public CountryResult {} } public record GeolocationResult(String city, String country, Double lat, Double lng, String postalCode, String region, String timezone) implements - IpDatabase.Response { + IpDataLookup.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public GeolocationResult( @@ -237,7 +237,7 @@ public GeolocationResult( public record PrivacyDetectionResult(Boolean hosting, Boolean proxy, Boolean relay, String service, Boolean tor, Boolean vpn) implements - IpDatabase.Response { + IpDataLookup.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public PrivacyDetectionResult( @@ -462,7 +462,7 @@ protected Map transform(final Result res * * @param the record type that will be wrapped and returned */ - private abstract static class AbstractBase implements IpDataLookup { + private abstract static class AbstractBase implements IpDataLookup { protected final Set properties; protected final Class clazz; @@ -479,7 +479,7 @@ public Set getProperties() { @Override public final Map getData(final IpDatabase ipDatabase, final String ipAddress) { - final Result response = ipDatabase.getTypedResponse(ipAddress, this::lookup); + final Result response = databaseLookup(ipDatabase, ipAddress, this::lookup); return (response == null || response.result() == null) ? Map.of() : transform(response); } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index b76d864594fed..5b1b69c26a098 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -109,7 +109,7 @@ static Function, IpDataLookup> getMaxmindLookup(final Dat }; } - static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements IpDatabase.Response { + static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements IpDataLookup.Response { CacheableAnonymousIpResponse(AnonymousIpResponse response) { super( @@ -176,7 +176,7 @@ protected Map transform(final CacheableAnonymousIpResponse respo } } - static class CacheableAsnResponse extends AsnResponse implements IpDatabase.Response { + static class CacheableAsnResponse extends AsnResponse implements IpDataLookup.Response { CacheableAsnResponse(AsnResponse response) { super( @@ -246,7 +246,7 @@ record CacheableCityResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements IpDatabase.Response {} + ) implements IpDataLookup.Response {} static class City extends AbstractBase> { City(final Set properties) { @@ -382,7 +382,7 @@ protected Map transform(final Result resu } } - static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements IpDatabase.Response { + static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements IpDataLookup.Response { CacheableConnectionTypeResponse(ConnectionTypeResponse response) { super(response.getConnectionType(), response.getIpAddress(), response.getNetwork()); @@ -431,7 +431,7 @@ record CacheableCountryResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements IpDatabase.Response {} + ) implements IpDataLookup.Response {} static class Country extends AbstractBase> { Country(final Set properties) { @@ -514,7 +514,7 @@ protected Map transform(final Result r } } - static class CacheableDomainResponse extends DomainResponse implements IpDatabase.Response { + static class CacheableDomainResponse extends DomainResponse implements IpDataLookup.Response { CacheableDomainResponse(DomainResponse response) { super(response.getDomain(), response.getIpAddress(), response.getNetwork()); @@ -589,7 +589,7 @@ record CacheableEnterpriseResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements IpDatabase.Response {} + ) implements IpDataLookup.Response {} static class Enterprise extends AbstractBase> { Enterprise(final Set properties) { @@ -826,7 +826,7 @@ protected Map transform(final Result { * * @param the intermediate type of {@link AbstractResponse} */ - private abstract static class AbstractBase + private abstract static class AbstractBase implements IpDataLookup { @@ -938,7 +938,7 @@ public Set getProperties() { @Override public final Map getData(final IpDatabase ipDatabase, final String ipAddress) { - final RECORD response = ipDatabase.getTypedResponse(ipAddress, this::lookup); + final RECORD response = databaseLookup(ipDatabase, ipAddress, this::lookup); return (response == null) ? Map.of() : transform(response); } diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java index 37f4d72927053..38a910c88cc99 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/ConfigDatabasesTests.java @@ -124,7 +124,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - GeoIpTestUtils.SimpleCity cityResponse = loader.getTypedResponse("89.160.20.128", GeoIpTestUtils::getCity); + GeoIpTestUtils.SimpleCity cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Tumba")); assertThat(cache.count(), equalTo(1)); } @@ -136,7 +136,7 @@ public void testDatabasesUpdateExistingConfDatabase() throws Exception { DatabaseReaderLazyLoader loader = configDatabases.getDatabase("GeoLite2-City.mmdb"); assertThat(loader.getDatabaseType(), equalTo("GeoLite2-City")); - GeoIpTestUtils.SimpleCity cityResponse = loader.getTypedResponse("89.160.20.128", GeoIpTestUtils::getCity); + GeoIpTestUtils.SimpleCity cityResponse = loader.getResponse("89.160.20.128", GeoIpTestUtils::getCity); assertThat(cityResponse.cityName(), equalTo("Linköping")); assertThat(cache.count(), equalTo(1)); }); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index 7b3ff0e2865b5..1a44e568490eb 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -25,7 +25,7 @@ public class GeoIpCacheTests extends ESTestCase { - private record FakeResponse(long sizeInBytes) implements IpDatabase.Response {} + private record FakeResponse(long sizeInBytes) implements IpDataLookup.Response {} public void testCachesAndEvictsResults_maxCount() { GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 93aa37ad8d886..90f4c94c8bfa8 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -67,7 +67,7 @@ static void copyDefaultDatabases(final Path directory, ConfigDatabases configDat } } - public record SimpleCity(String cityName) implements IpDatabase.Response {} + public record SimpleCity(String cityName) implements IpDataLookup.Response {} /** * A static city-specific responseProvider for use with {@link IpDatabase#getTypedResponse(String, CheckedBiFunction)} in @@ -81,7 +81,7 @@ public static SimpleCity getCity(Reader reader, String ip) throws IOException { return data == null ? null : new SimpleCity(new CityResponse(data, ip, record.getNetwork(), List.of("en")).getCity().getName()); } - public record SimpleCountry(String countryName) implements IpDatabase.Response {} + public record SimpleCountry(String countryName) implements IpDataLookup.Response {} /** * A static country-specific responseProvider for use with {@link IpDatabase#getTypedResponse(String, CheckedBiFunction)} in From 24601ac26c1149fe65f8bd0ddd667e6e0c0906c1 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Thu, 10 Jul 2025 20:50:08 +0100 Subject: [PATCH 17/19] Try a third version --- .../geoip/DatabaseReaderLazyLoader.java | 25 +++++++++---------- .../ingest/geoip/GeoIpCache.java | 24 ++++++++++++------ .../ingest/geoip/IpDataLookup.java | 18 ++++++------- .../ingest/geoip/IpinfoIpDataLookups.java | 10 ++++---- .../ingest/geoip/MaxmindIpDataLookups.java | 18 ++++++------- .../ingest/geoip/GeoIpCacheTests.java | 2 +- .../ingest/geoip/GeoIpTestUtils.java | 4 +-- 7 files changed, 52 insertions(+), 49 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java index 9e324f4ffc217..7f82c4037fd2c 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseReaderLazyLoader.java @@ -30,13 +30,12 @@ import java.nio.file.Path; import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Function; /** * Facilitates lazy loading of the database reader, so that when the geoip plugin is installed, but not used, * no memory is being wasted on the database reader. */ -public class DatabaseReaderLazyLoader implements IpDatabase { +public class DatabaseReaderLazyLoader implements CacheingIpDatabase { private static final boolean LOAD_DATABASE_ON_HEAP = Booleans.parseBoolean(System.getProperty("es.geoip.load_db_on_heap", "false")); @@ -117,22 +116,22 @@ int current() { @Override @Nullable public RESPONSE getResponse(String ipAddress, CheckedBiFunction responseProvider) { - Function retrieveFunction = ip -> { + // TODO(pete): Improve the documentation here. + throw new UnsupportedOperationException("This is a CacheingIpDatabase, callers should use get getCacheableResponse instead"); + } + + @Nullable + public RESPONSE getCacheableResponse( + String ipAddress, + CheckedBiFunction responseProvider + ) { + return cache.putIfAbsent(projectId, ipAddress, cachedDatabasePathToString, ip -> { try { return responseProvider.apply(get(), ipAddress); } catch (Exception e) { throw ExceptionsHelper.convertToRuntime(e); } - }; - @SuppressWarnings("unchecked") // This is not generally safe! However, it is okay since we know that we will only call getResponse - // with RESPONSE types which extend IpDataLookup.Response. TODO(pete): Explain why we're doing this! - Function castRetrieveFunction = (Function< - String, - ? extends IpDataLookup.Response>) retrieveFunction; - @SuppressWarnings("unchecked") // This one *is* safe, because we know that the retrieve function really does return RESPONSE (it was - // typed as such before we cast it). - RESPONSE response = (RESPONSE) cache.putIfAbsent(projectId, ipAddress, cachedDatabasePathToString, castRetrieveFunction); - return response; + }); } Reader get() throws IOException { diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index c8d7171697e5f..bffe567eb77ca 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -35,7 +35,7 @@ static GeoIpCache createGeoIpCacheWithMaxCount(long maxSize) { if (maxSize < 0) { throw new IllegalArgumentException("geoip max cache size must be 0 or greater"); } - return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); + return new GeoIpCache(System::nanoTime, CacheBuilder.builder().setMaximumWeight(maxSize).build()); } static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { @@ -44,7 +44,7 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { } return new GeoIpCache( System::nanoTime, - CacheBuilder.builder() + CacheBuilder.builder() .setMaximumWeight(maxByteSize.getBytes()) .weigher((key, value) -> key.sizeInBytes() + value.sizeInBytes()) .build() @@ -55,7 +55,7 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize, LongSupplier relativeNanoTimeProvider) { return new GeoIpCache( relativeNanoTimeProvider, - CacheBuilder.builder().setMaximumWeight(maxSize).build() + CacheBuilder.builder().setMaximumWeight(maxSize).build() ); } @@ -65,7 +65,7 @@ static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize * something not being in the cache because the data doesn't exist in the database. */ // visible for testing - static final IpDataLookup.Response NO_RESULT = new IpDataLookup.Response() { + static final Response NO_RESULT = new Response() { @Override public String toString() { return "NO_RESULT"; @@ -73,17 +73,17 @@ public String toString() { }; private final LongSupplier relativeNanoTimeProvider; - private final Cache cache; + private final Cache cache; private final AtomicLong hitsTimeInNanos = new AtomicLong(0); private final AtomicLong missesTimeInNanos = new AtomicLong(0); - private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { + private GeoIpCache(LongSupplier relativeNanoTimeProvider, Cache cache) { this.relativeNanoTimeProvider = relativeNanoTimeProvider; this.cache = cache; } @SuppressWarnings("unchecked") - RESPONSE putIfAbsent( + RESPONSE putIfAbsent( ProjectId projectId, String ip, String databasePath, @@ -93,7 +93,7 @@ RESPONSE putIfAbsent( CacheKey cacheKey = new CacheKey(projectId, ip, databasePath); long cacheStart = relativeNanoTimeProvider.getAsLong(); // intentionally non-locking for simplicity...it's OK if we re-put the same key/value in the cache during a race condition. - IpDataLookup.Response response = cache.get(cacheKey); + Response response = cache.get(cacheKey); long cacheRequestTime = relativeNanoTimeProvider.getAsLong() - cacheStart; // populate the cache for this key, if necessary @@ -158,6 +158,14 @@ public CacheStats getCacheStats() { ); } + public interface Response { + + // TODO PETE: Remove this default implementation and implement in all implementing classes instead + default long sizeInBytes() { + return 0; + } + } + /** * The key to use for the cache. Since this cache can span multiple geoip processors that all use different databases, the database * 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 diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java index 108fab921e31b..8410fd4a808f0 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpDataLookup.java @@ -28,12 +28,16 @@ interface IpDataLookup { */ Map getData(IpDatabase ipDatabase, String ip) throws IOException; - default RECORD databaseLookup( + default RECORD databaseLookup( IpDatabase ipDatabase, String ipAddress, CheckedBiFunction responseProvider ) { - return ipDatabase.getResponse(ipAddress, responseProvider); + if (ipDatabase instanceof CacheingIpDatabase cacheingIpDatabase) { + return cacheingIpDatabase.getCacheableResponse(ipAddress, responseProvider); + } else { + return ipDatabase.getResponse(ipAddress, responseProvider); + } } /** @@ -41,18 +45,10 @@ default RECORD databaseLookup( */ Set getProperties(); - interface Response { - - // TODO PETE: Remove this default implementation and implement in all implementing classes instead - default long sizeInBytes() { - return 0; - } - } - /** * A helper record that holds other records. Every ip data lookup will have an associated ip address that was looked up, as well * as a network for which the record applies. Having a helper record prevents each individual response record from needing to * track these bits of information. */ - record Result(T result, String ip, String network) implements Response {} + record Result(T result, String ip, String network) implements GeoIpCache.Response {} } diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index a859e533aa7a8..5b085236e205b 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -191,7 +191,7 @@ public record AsnResult( String domain, String name, @Nullable String type // not present in the free asn database - ) implements IpDataLookup.Response { + ) implements GeoIpCache.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public AsnResult( @@ -210,14 +210,14 @@ public record CountryResult( @MaxMindDbParameter(name = "continent_name") String continentName, @MaxMindDbParameter(name = "country") String country, @MaxMindDbParameter(name = "country_name") String countryName - ) implements IpDataLookup.Response { + ) implements GeoIpCache.Response { @MaxMindDbConstructor public CountryResult {} } public record GeolocationResult(String city, String country, Double lat, Double lng, String postalCode, String region, String timezone) implements - IpDataLookup.Response { + GeoIpCache.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public GeolocationResult( @@ -237,7 +237,7 @@ public GeolocationResult( public record PrivacyDetectionResult(Boolean hosting, Boolean proxy, Boolean relay, String service, Boolean tor, Boolean vpn) implements - IpDataLookup.Response { + GeoIpCache.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public PrivacyDetectionResult( @@ -462,7 +462,7 @@ protected Map transform(final Result res * * @param the record type that will be wrapped and returned */ - private abstract static class AbstractBase implements IpDataLookup { + private abstract static class AbstractBase implements IpDataLookup { protected final Set properties; protected final Class clazz; diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java index 5b1b69c26a098..6408a3c9916a3 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/MaxmindIpDataLookups.java @@ -109,7 +109,7 @@ static Function, IpDataLookup> getMaxmindLookup(final Dat }; } - static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements IpDataLookup.Response { + static class CacheableAnonymousIpResponse extends AnonymousIpResponse implements GeoIpCache.Response { CacheableAnonymousIpResponse(AnonymousIpResponse response) { super( @@ -176,7 +176,7 @@ protected Map transform(final CacheableAnonymousIpResponse respo } } - static class CacheableAsnResponse extends AsnResponse implements IpDataLookup.Response { + static class CacheableAsnResponse extends AsnResponse implements GeoIpCache.Response { CacheableAsnResponse(AsnResponse response) { super( @@ -246,7 +246,7 @@ record CacheableCityResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements IpDataLookup.Response {} + ) implements GeoIpCache.Response {} static class City extends AbstractBase> { City(final Set properties) { @@ -382,7 +382,7 @@ protected Map transform(final Result resu } } - static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements IpDataLookup.Response { + static class CacheableConnectionTypeResponse extends ConnectionTypeResponse implements GeoIpCache.Response { CacheableConnectionTypeResponse(ConnectionTypeResponse response) { super(response.getConnectionType(), response.getIpAddress(), response.getNetwork()); @@ -431,7 +431,7 @@ record CacheableCountryResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements IpDataLookup.Response {} + ) implements GeoIpCache.Response {} static class Country extends AbstractBase> { Country(final Set properties) { @@ -514,7 +514,7 @@ protected Map transform(final Result r } } - static class CacheableDomainResponse extends DomainResponse implements IpDataLookup.Response { + static class CacheableDomainResponse extends DomainResponse implements GeoIpCache.Response { CacheableDomainResponse(DomainResponse response) { super(response.getDomain(), response.getIpAddress(), response.getNetwork()); @@ -589,7 +589,7 @@ record CacheableEnterpriseResponse( Boolean registeredCountryIsInEuropeanUnion, String registeredCountryIsoCode, String registeredCountryName - ) implements IpDataLookup.Response {} + ) implements GeoIpCache.Response {} static class Enterprise extends AbstractBase> { Enterprise(final Set properties) { @@ -826,7 +826,7 @@ protected Map transform(final Result { * * @param the intermediate type of {@link AbstractResponse} */ - private abstract static class AbstractBase + private abstract static class AbstractBase implements IpDataLookup { diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java index 1a44e568490eb..232f366781ac6 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpCacheTests.java @@ -25,7 +25,7 @@ public class GeoIpCacheTests extends ESTestCase { - private record FakeResponse(long sizeInBytes) implements IpDataLookup.Response {} + private record FakeResponse(long sizeInBytes) implements GeoIpCache.Response {} public void testCachesAndEvictsResults_maxCount() { GeoIpCache cache = GeoIpCache.createGeoIpCacheWithMaxCount(1); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java index 90f4c94c8bfa8..09d81be13fe42 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpTestUtils.java @@ -67,7 +67,7 @@ static void copyDefaultDatabases(final Path directory, ConfigDatabases configDat } } - public record SimpleCity(String cityName) implements IpDataLookup.Response {} + public record SimpleCity(String cityName) implements GeoIpCache.Response {} /** * A static city-specific responseProvider for use with {@link IpDatabase#getTypedResponse(String, CheckedBiFunction)} in @@ -81,7 +81,7 @@ public static SimpleCity getCity(Reader reader, String ip) throws IOException { return data == null ? null : new SimpleCity(new CityResponse(data, ip, record.getNetwork(), List.of("en")).getCity().getName()); } - public record SimpleCountry(String countryName) implements IpDataLookup.Response {} + public record SimpleCountry(String countryName) implements GeoIpCache.Response {} /** * A static country-specific responseProvider for use with {@link IpDatabase#getTypedResponse(String, CheckedBiFunction)} in From b70b110dae9131aa5be17cece60752ddaf264004 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 10 Jul 2025 19:59:29 +0000 Subject: [PATCH 18/19] [CI] Auto commit changes from spotless --- .../main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java | 5 +---- .../org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java index bffe567eb77ca..2b3800e98ee77 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java @@ -53,10 +53,7 @@ static GeoIpCache createGeoIpCacheWithMaxBytes(ByteSizeValue maxByteSize) { // package private for testing static GeoIpCache createGeoIpCacheWithMaxCountAndCustomTimeProvider(long maxSize, LongSupplier relativeNanoTimeProvider) { - return new GeoIpCache( - relativeNanoTimeProvider, - CacheBuilder.builder().setMaximumWeight(maxSize).build() - ); + return new GeoIpCache(relativeNanoTimeProvider, CacheBuilder.builder().setMaximumWeight(maxSize).build()); } /** diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java index 5b085236e205b..201fa60c9b900 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IpinfoIpDataLookups.java @@ -217,7 +217,7 @@ public record CountryResult( public record GeolocationResult(String city, String country, Double lat, Double lng, String postalCode, String region, String timezone) implements - GeoIpCache.Response { + GeoIpCache.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public GeolocationResult( @@ -237,7 +237,7 @@ public GeolocationResult( public record PrivacyDetectionResult(Boolean hosting, Boolean proxy, Boolean relay, String service, Boolean tor, Boolean vpn) implements - GeoIpCache.Response { + GeoIpCache.Response { @SuppressWarnings("checkstyle:RedundantModifier") @MaxMindDbConstructor public PrivacyDetectionResult( From 376f681ea0e4c5e06c7c8eb05b3d3051b73970e3 Mon Sep 17 00:00:00 2001 From: Pete Gillin Date: Wed, 16 Jul 2025 15:44:15 +0100 Subject: [PATCH 19/19] Include file I forgot to add --- .../ingest/geoip/CacheingIpDatabase.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/CacheingIpDatabase.java diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/CacheingIpDatabase.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/CacheingIpDatabase.java new file mode 100644 index 0000000000000..d5c584a4dd096 --- /dev/null +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/CacheingIpDatabase.java @@ -0,0 +1,24 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +package org.elasticsearch.ingest.geoip; + +import com.maxmind.db.Reader; + +import org.elasticsearch.common.CheckedBiFunction; +import org.elasticsearch.core.Nullable; + +public interface CacheingIpDatabase extends IpDatabase { + + @Nullable + RESPONSE getCacheableResponse( + String ipAddress, + CheckedBiFunction responseProvider + ); +}