From 1da9227d81a935379424fb136b42225942bb6e37 Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Tue, 9 Jul 2024 10:24:09 -0500 Subject: [PATCH 1/5] Adding a unit test for GeoIpDownloader.cleanDatabases --- .../ingest/geoip/GeoIpDownloader.java | 10 ++ .../ingest/geoip/GeoIpDownloaderTests.java | 112 +++++++++++++++++- .../persistent/PersistentTasksService.java | 2 +- 3 files changed, 122 insertions(+), 2 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java index 5239e96856b7f..3b043317100be 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java @@ -33,6 +33,7 @@ import org.elasticsearch.ingest.geoip.stats.GeoIpDownloaderStats; import org.elasticsearch.persistent.AllocatedPersistentTask; import org.elasticsearch.persistent.PersistentTasksCustomMetadata.PersistentTask; +import org.elasticsearch.persistent.PersistentTasksService; import org.elasticsearch.tasks.TaskId; import org.elasticsearch.threadpool.Scheduler; import org.elasticsearch.threadpool.ThreadPool; @@ -349,6 +350,15 @@ public GeoIpDownloaderStats getStatus() { return isCancelled() || isCompleted() ? null : stats; } + /** + * This sets the value of this GeoIpDownloader's persistentTasksService to the given value. It is meant for unit tests only because + * it does not initialize any of several other fields that need to be initialized in real usage. + * @param persistentTasksService The test PersistentTasksService to use + */ + void setPersistentTasksService(PersistentTasksService persistentTasksService) { + init(persistentTasksService, null, null, 0); + } + private void scheduleNextRun(TimeValue time) { if (threadPool.scheduler().isShutdown() == false) { scheduled = threadPool.schedule(this::runDownloader, time, threadPool.generic()); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java index 4834c581e9386..0cacfc610aff8 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java @@ -23,6 +23,7 @@ import org.elasticsearch.action.index.IndexResponse; import org.elasticsearch.action.index.TransportIndexAction; import org.elasticsearch.action.support.broadcast.BroadcastResponse; +import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlocks; @@ -30,11 +31,18 @@ import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.TimeValue; +import org.elasticsearch.index.reindex.BulkByScrollResponse; +import org.elasticsearch.index.reindex.DeleteByQueryAction; +import org.elasticsearch.index.reindex.DeleteByQueryRequest; import org.elasticsearch.ingest.geoip.stats.GeoIpDownloaderStats; import org.elasticsearch.node.Node; +import org.elasticsearch.persistent.PersistentTaskParams; import org.elasticsearch.persistent.PersistentTaskState; import org.elasticsearch.persistent.PersistentTasksCustomMetadata; import org.elasticsearch.persistent.PersistentTasksCustomMetadata.PersistentTask; +import org.elasticsearch.persistent.PersistentTasksService; import org.elasticsearch.telemetry.metric.MeterRegistry; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.client.NoOpClient; @@ -49,6 +57,9 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.time.temporal.ChronoUnit; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -63,6 +74,8 @@ import static org.elasticsearch.ingest.geoip.GeoIpDownloader.MAX_CHUNK_SIZE; import static org.elasticsearch.tasks.TaskId.EMPTY_TASK_ID; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -76,8 +89,9 @@ public class GeoIpDownloaderTests extends ESTestCase { private GeoIpDownloader geoIpDownloader; @Before - public void setup() { + public void setup() throws IOException { httpClient = mock(HttpClient.class); + when(httpClient.getBytes(anyString())).thenReturn("[]".getBytes(StandardCharsets.UTF_8)); clusterService = mock(ClusterService.class); threadPool = new ThreadPool(Settings.builder().put(Node.NODE_NAME_SETTING.getKey(), "test").build(), MeterRegistry.NOOP); when(clusterService.getClusterSettings()).thenReturn( @@ -541,6 +555,61 @@ public void testUpdateDatabasesIndexNotReady() { verifyNoInteractions(httpClient); } + public void testThatRunDownloaderDeletesExpiredDatabases() { + /* + * This test puts some expired databases and some non-expired ones into the GeoIpTaskState, and then calls runDownloader(), making + * sure that the expired databases have been deleted. + */ + AtomicInteger deleteCount = new AtomicInteger(0); + int expiredDatabasesCount = randomIntBetween(1, 100); + int unexpiredDatabasesCount = randomIntBetween(0, 100); + Map databases = new HashMap<>(); + for (int i = 0; i < expiredDatabasesCount; i++) { + databases.put( + "expiredDatabase" + i, + new GeoIpTaskState.Metadata(0, 0, 0, randomAlphaOfLength(20), Instant.now().minus(40, ChronoUnit.DAYS).toEpochMilli()) + ); + } + for (int i = 0; i < unexpiredDatabasesCount; i++) { + databases.put( + "unexpiredDatabase" + i, + new GeoIpTaskState.Metadata( + 0, + 0, + 0, + randomAlphaOfLength(20), + Instant.now().minus(randomIntBetween(0, 29), ChronoUnit.DAYS).toEpochMilli() + ) + ); + } + GeoIpTaskState geoIpTaskState = new GeoIpTaskState(databases); + geoIpDownloader.setState(geoIpTaskState); + client.addHandler( + DeleteByQueryAction.INSTANCE, + (DeleteByQueryRequest request, ActionListener flushResponseActionListener) -> { + deleteCount.incrementAndGet(); + } + ); + GeoIpTaskParams geoIpTaskParams = mock(GeoIpTaskParams.class); + when(geoIpTaskParams.getWriteableName()).thenReturn(GeoIpDownloader.GEOIP_DOWNLOADER); + geoIpDownloader.setPersistentTasksService( + new TestPersistentTasksService(clusterService, threadPool, client, GeoIpDownloader.GEOIP_DOWNLOADER, geoIpTaskParams) + ); + geoIpDownloader.runDownloader(); + assertThat(geoIpDownloader.getStatus().getExpiredDatabases(), equalTo(expiredDatabasesCount)); + for (int i = 0; i < expiredDatabasesCount; i++) { + // This currently fails because we subtract one millisecond from the lastChecked time + // assertThat(geoIpDownloader.state.getDatabases().get("expiredDatabase" + i).lastCheck(), equalTo(-1L)); + } + for (int i = 0; i < unexpiredDatabasesCount; i++) { + assertThat( + geoIpDownloader.state.getDatabases().get("unexpiredDatabase" + i).lastCheck(), + greaterThanOrEqualTo(Instant.now().minus(30, ChronoUnit.DAYS).toEpochMilli()) + ); + } + assertThat(deleteCount.get(), equalTo(expiredDatabasesCount)); + } + private static class MockClient extends NoOpClient { private final Map, BiConsumer>> handlers = new HashMap<>(); @@ -573,4 +642,45 @@ protected void } } } + + /* + * This is a test implementation of PersistentTasksService that overrides sendUpdateStateRequest to immediately notify its listener of + * success, rather than sending the request over the wire. + */ + private static class TestPersistentTasksService extends PersistentTasksService { + + private final String taskName; + private final PersistentTaskParams persistentTaskParams; + + TestPersistentTasksService( + ClusterService clusterService, + ThreadPool threadPool, + Client client, + String taskName, + PersistentTaskParams persistentTaskParams + ) { + super(clusterService, threadPool, client); + this.taskName = taskName; + this.persistentTaskParams = persistentTaskParams; + } + + @Override + protected void sendUpdateStateRequest( + final String taskId, + final long taskAllocationID, + final PersistentTaskState taskState, + final @Nullable TimeValue timeout, + final ActionListener> listener + ) { + PersistentTasksCustomMetadata.Assignment assignment = mock(PersistentTasksCustomMetadata.Assignment.class); + PersistentTasksCustomMetadata.PersistentTask persistentTask = new PersistentTasksCustomMetadata.PersistentTask<>( + taskId, + taskName, + persistentTaskParams, + taskAllocationID, + assignment + ); + listener.onResponse(new PersistentTasksCustomMetadata.PersistentTask<>(persistentTask, taskState)); + } + } } diff --git a/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java b/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java index 3ea4b19f77a45..46d4ae491acd4 100644 --- a/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java +++ b/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java @@ -125,7 +125,7 @@ void sendCancelRequest( * {@link AllocatedPersistentTask#updatePersistentTaskState} instead. * Accepts operation timeout as optional parameter */ - void sendUpdateStateRequest( + protected void sendUpdateStateRequest( final String taskId, final long taskAllocationID, final PersistentTaskState taskState, From 6483d54a227aea143620be50627afcc6dbf77d03 Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Tue, 9 Jul 2024 10:31:08 -0500 Subject: [PATCH 2/5] pulling metadata creation into a method --- .../ingest/geoip/GeoIpDownloaderTests.java | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java index 0cacfc610aff8..37dcdd5065b6d 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java @@ -565,22 +565,10 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { int unexpiredDatabasesCount = randomIntBetween(0, 100); Map databases = new HashMap<>(); for (int i = 0; i < expiredDatabasesCount; i++) { - databases.put( - "expiredDatabase" + i, - new GeoIpTaskState.Metadata(0, 0, 0, randomAlphaOfLength(20), Instant.now().minus(40, ChronoUnit.DAYS).toEpochMilli()) - ); + databases.put("expiredDatabase" + i, newGeoIpTaskStateMetadata(true)); } for (int i = 0; i < unexpiredDatabasesCount; i++) { - databases.put( - "unexpiredDatabase" + i, - new GeoIpTaskState.Metadata( - 0, - 0, - 0, - randomAlphaOfLength(20), - Instant.now().minus(randomIntBetween(0, 29), ChronoUnit.DAYS).toEpochMilli() - ) - ); + databases.put("unexpiredDatabase" + i, newGeoIpTaskStateMetadata(false)); } GeoIpTaskState geoIpTaskState = new GeoIpTaskState(databases); geoIpDownloader.setState(geoIpTaskState); @@ -610,6 +598,16 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { assertThat(deleteCount.get(), equalTo(expiredDatabasesCount)); } + private GeoIpTaskState.Metadata newGeoIpTaskStateMetadata(boolean expired) { + Instant lastChecked; + if (expired) { + lastChecked = Instant.now().minus(randomIntBetween(31, 100), ChronoUnit.DAYS); + } else { + lastChecked = Instant.now().minus(randomIntBetween(0, 29), ChronoUnit.DAYS); + } + return new GeoIpTaskState.Metadata(0, 0, 0, randomAlphaOfLength(20), lastChecked.toEpochMilli()); + } + private static class MockClient extends NoOpClient { private final Map, BiConsumer>> handlers = new HashMap<>(); From 6e96c30a2db8649db4a291325ef34861b8fb3794 Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Tue, 9 Jul 2024 10:51:40 -0500 Subject: [PATCH 3/5] removing GeoIpDownloader::setPersistentTaskSerivce --- .../ingest/geoip/GeoIpDownloader.java | 10 ---------- .../ingest/geoip/GeoIpDownloaderTests.java | 18 ++++++++++++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java index 3b043317100be..5239e96856b7f 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java @@ -33,7 +33,6 @@ import org.elasticsearch.ingest.geoip.stats.GeoIpDownloaderStats; import org.elasticsearch.persistent.AllocatedPersistentTask; import org.elasticsearch.persistent.PersistentTasksCustomMetadata.PersistentTask; -import org.elasticsearch.persistent.PersistentTasksService; import org.elasticsearch.tasks.TaskId; import org.elasticsearch.threadpool.Scheduler; import org.elasticsearch.threadpool.ThreadPool; @@ -350,15 +349,6 @@ public GeoIpDownloaderStats getStatus() { return isCancelled() || isCompleted() ? null : stats; } - /** - * This sets the value of this GeoIpDownloader's persistentTasksService to the given value. It is meant for unit tests only because - * it does not initialize any of several other fields that need to be initialized in real usage. - * @param persistentTasksService The test PersistentTasksService to use - */ - void setPersistentTasksService(PersistentTasksService persistentTasksService) { - init(persistentTasksService, null, null, 0); - } - private void scheduleNextRun(TimeValue time) { if (threadPool.scheduler().isShutdown() == false) { scheduled = threadPool.schedule(this::runDownloader, time, threadPool.generic()); diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java index 37dcdd5065b6d..06ea0680b60c5 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java @@ -123,7 +123,18 @@ public void setup() throws IOException { () -> GeoIpDownloaderTaskExecutor.POLL_INTERVAL_SETTING.getDefault(Settings.EMPTY), () -> GeoIpDownloaderTaskExecutor.EAGER_DOWNLOAD_SETTING.getDefault(Settings.EMPTY), () -> true - ); + ) { + { + GeoIpTaskParams geoIpTaskParams = mock(GeoIpTaskParams.class); + when(geoIpTaskParams.getWriteableName()).thenReturn(GeoIpDownloader.GEOIP_DOWNLOADER); + init( + new TestPersistentTasksService(clusterService, threadPool, client, GeoIpDownloader.GEOIP_DOWNLOADER, geoIpTaskParams), + null, + null, + 0 + ); + } + }; } @After @@ -578,11 +589,6 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { deleteCount.incrementAndGet(); } ); - GeoIpTaskParams geoIpTaskParams = mock(GeoIpTaskParams.class); - when(geoIpTaskParams.getWriteableName()).thenReturn(GeoIpDownloader.GEOIP_DOWNLOADER); - geoIpDownloader.setPersistentTasksService( - new TestPersistentTasksService(clusterService, threadPool, client, GeoIpDownloader.GEOIP_DOWNLOADER, geoIpTaskParams) - ); geoIpDownloader.runDownloader(); assertThat(geoIpDownloader.getStatus().getExpiredDatabases(), equalTo(expiredDatabasesCount)); for (int i = 0; i < expiredDatabasesCount; i++) { From 1740b94dff4efc3864cd48d6ea99465010692c86 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Tue, 9 Jul 2024 12:23:17 -0400 Subject: [PATCH 4/5] Drop TestPersistentTasksService in favor of an addHandler --- .../ingest/geoip/GeoIpDownloaderTests.java | 69 +++++-------------- .../persistent/PersistentTasksService.java | 2 +- 2 files changed, 19 insertions(+), 52 deletions(-) diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java index 06ea0680b60c5..675c55f7e7cf2 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java @@ -23,7 +23,6 @@ import org.elasticsearch.action.index.IndexResponse; import org.elasticsearch.action.index.TransportIndexAction; import org.elasticsearch.action.support.broadcast.BroadcastResponse; -import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlocks; @@ -31,18 +30,17 @@ import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.core.Nullable; -import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.reindex.BulkByScrollResponse; import org.elasticsearch.index.reindex.DeleteByQueryAction; import org.elasticsearch.index.reindex.DeleteByQueryRequest; import org.elasticsearch.ingest.geoip.stats.GeoIpDownloaderStats; import org.elasticsearch.node.Node; -import org.elasticsearch.persistent.PersistentTaskParams; +import org.elasticsearch.persistent.PersistentTaskResponse; import org.elasticsearch.persistent.PersistentTaskState; import org.elasticsearch.persistent.PersistentTasksCustomMetadata; import org.elasticsearch.persistent.PersistentTasksCustomMetadata.PersistentTask; import org.elasticsearch.persistent.PersistentTasksService; +import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.telemetry.metric.MeterRegistry; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.client.NoOpClient; @@ -127,12 +125,7 @@ public void setup() throws IOException { { GeoIpTaskParams geoIpTaskParams = mock(GeoIpTaskParams.class); when(geoIpTaskParams.getWriteableName()).thenReturn(GeoIpDownloader.GEOIP_DOWNLOADER); - init( - new TestPersistentTasksService(clusterService, threadPool, client, GeoIpDownloader.GEOIP_DOWNLOADER, geoIpTaskParams), - null, - null, - 0 - ); + init(new PersistentTasksService(clusterService, threadPool, client), null, null, 0); } }; } @@ -583,6 +576,21 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { } GeoIpTaskState geoIpTaskState = new GeoIpTaskState(databases); geoIpDownloader.setState(geoIpTaskState); + client.addHandler( + UpdatePersistentTaskStatusAction.INSTANCE, + (UpdatePersistentTaskStatusAction.Request request, ActionListener taskResponseListener) -> { + + PersistentTasksCustomMetadata.Assignment assignment = mock(PersistentTasksCustomMetadata.Assignment.class); + PersistentTasksCustomMetadata.PersistentTask persistentTask = new PersistentTasksCustomMetadata.PersistentTask<>( + GeoIpDownloader.GEOIP_DOWNLOADER, + GeoIpDownloader.GEOIP_DOWNLOADER, + new GeoIpTaskParams(), + request.getAllocationId(), + assignment + ); + taskResponseListener.onResponse(new PersistentTaskResponse(new PersistentTask<>(persistentTask, request.getState()))); + } + ); client.addHandler( DeleteByQueryAction.INSTANCE, (DeleteByQueryRequest request, ActionListener flushResponseActionListener) -> { @@ -646,45 +654,4 @@ protected void } } } - - /* - * This is a test implementation of PersistentTasksService that overrides sendUpdateStateRequest to immediately notify its listener of - * success, rather than sending the request over the wire. - */ - private static class TestPersistentTasksService extends PersistentTasksService { - - private final String taskName; - private final PersistentTaskParams persistentTaskParams; - - TestPersistentTasksService( - ClusterService clusterService, - ThreadPool threadPool, - Client client, - String taskName, - PersistentTaskParams persistentTaskParams - ) { - super(clusterService, threadPool, client); - this.taskName = taskName; - this.persistentTaskParams = persistentTaskParams; - } - - @Override - protected void sendUpdateStateRequest( - final String taskId, - final long taskAllocationID, - final PersistentTaskState taskState, - final @Nullable TimeValue timeout, - final ActionListener> listener - ) { - PersistentTasksCustomMetadata.Assignment assignment = mock(PersistentTasksCustomMetadata.Assignment.class); - PersistentTasksCustomMetadata.PersistentTask persistentTask = new PersistentTasksCustomMetadata.PersistentTask<>( - taskId, - taskName, - persistentTaskParams, - taskAllocationID, - assignment - ); - listener.onResponse(new PersistentTasksCustomMetadata.PersistentTask<>(persistentTask, taskState)); - } - } } diff --git a/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java b/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java index 46d4ae491acd4..3ea4b19f77a45 100644 --- a/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java +++ b/server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java @@ -125,7 +125,7 @@ void sendCancelRequest( * {@link AllocatedPersistentTask#updatePersistentTaskState} instead. * Accepts operation timeout as optional parameter */ - protected void sendUpdateStateRequest( + void sendUpdateStateRequest( final String taskId, final long taskAllocationID, final PersistentTaskState taskState, From 0a7761f8ad8f46fe16301d125f123400154b1e3f Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Tue, 9 Jul 2024 12:58:01 -0500 Subject: [PATCH 5/5] asserting questionable current behavior --- .../ingest/geoip/GeoIpDownloaderTests.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java index 675c55f7e7cf2..4d5070d96683e 100644 --- a/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java +++ b/modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java @@ -564,6 +564,7 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { * This test puts some expired databases and some non-expired ones into the GeoIpTaskState, and then calls runDownloader(), making * sure that the expired databases have been deleted. */ + AtomicInteger updatePersistentTaskStateCount = new AtomicInteger(0); AtomicInteger deleteCount = new AtomicInteger(0); int expiredDatabasesCount = randomIntBetween(1, 100); int unexpiredDatabasesCount = randomIntBetween(0, 100); @@ -589,6 +590,7 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { assignment ); taskResponseListener.onResponse(new PersistentTaskResponse(new PersistentTask<>(persistentTask, request.getState()))); + updatePersistentTaskStateCount.incrementAndGet(); } ); client.addHandler( @@ -610,6 +612,14 @@ public void testThatRunDownloaderDeletesExpiredDatabases() { ); } assertThat(deleteCount.get(), equalTo(expiredDatabasesCount)); + assertThat(updatePersistentTaskStateCount.get(), equalTo(expiredDatabasesCount)); + geoIpDownloader.runDownloader(); + /* + * The following two lines assert current behavior that might not be desirable -- we continue to delete expired databases every + * time that runDownloader runs. This seems unnecessary. + */ + assertThat(deleteCount.get(), equalTo(expiredDatabasesCount * 2)); + assertThat(updatePersistentTaskStateCount.get(), equalTo(expiredDatabasesCount * 2)); } private GeoIpTaskState.Metadata newGeoIpTaskStateMetadata(boolean expired) {