From 66554b9ace35ebc86e54615819e5dbd7833e977d Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Wed, 2 Jul 2025 19:17:01 +0300 Subject: [PATCH] assertTrue for submitMergeTask (#130430) Previously, out of zealousness for testing efficiency, the mocked filesystems were reused across the test suite class. But this makes tests liable to interference wrt to filesystem stats calls. Moreover, if one test fails, it can trigger failures in other test methods. This PR recreates the mocked filesystems for every test method. Fixes #129296 #130205 --- muted-tests.yml | 3 - ...oolMergeExecutorServiceDiskSpaceTests.java | 88 ++++++------------- 2 files changed, 25 insertions(+), 66 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 474cd870c18d2..069fd22933231 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -485,9 +485,6 @@ tests: - class: org.elasticsearch.xpack.ml.integration.ClassificationIT method: testWithDatastreams issue: https://github.com/elastic/elasticsearch/issues/129457 -- class: org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests - method: testMergeTasksAreUnblockedWhenMoreDiskSpaceBecomesAvailable - issue: https://github.com/elastic/elasticsearch/issues/129296 - class: org.elasticsearch.xpack.profiling.action.GetStatusActionIT method: testWaitsUntilResourcesAreCreated issue: https://github.com/elastic/elasticsearch/issues/129486 diff --git a/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java b/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java index 8a497d09320ff..33a86ef5709ad 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java @@ -27,8 +27,7 @@ import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; import org.junit.After; -import org.junit.AfterClass; -import org.junit.BeforeClass; +import org.junit.Before; import java.io.IOException; import java.nio.file.FileStore; @@ -59,8 +58,8 @@ public class ThreadPoolMergeExecutorServiceDiskSpaceTests extends ESTestCase { - private static TestMockFileStore aFileStore = new TestMockFileStore("mocka"); - private static TestMockFileStore bFileStore = new TestMockFileStore("mockb"); + private static TestMockFileStore aFileStore; + private static TestMockFileStore bFileStore; private static String aPathPart; private static String bPathPart; private static int mergeExecutorThreadCount; @@ -69,8 +68,10 @@ public class ThreadPoolMergeExecutorServiceDiskSpaceTests extends ESTestCase { private static NodeEnvironment nodeEnvironment; private static boolean setThreadPoolMergeSchedulerSetting; - @BeforeClass - public static void installMockUsableSpaceFS() throws Exception { + @Before + public void setupTestEnv() throws Exception { + aFileStore = new TestMockFileStore("mocka"); + bFileStore = new TestMockFileStore("mockb"); FileSystem current = PathUtils.getDefaultFileSystem(); aPathPart = "a-" + randomUUID(); bPathPart = "b-" + randomUUID(); @@ -96,8 +97,14 @@ public static void installMockUsableSpaceFS() throws Exception { nodeEnvironment = new NodeEnvironment(settings, TestEnvironment.newEnvironment(settings)); } - @AfterClass - public static void removeMockUsableSpaceFS() { + @After + public void removeMockUsableSpaceFS() { + if (setThreadPoolMergeSchedulerSetting) { + assertWarnings( + "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " + + "and will be removed in a future release. See the breaking changes documentation for the next major version." + ); + } PathUtilsForTesting.teardown(); aFileStore = null; bFileStore = null; @@ -105,11 +112,6 @@ public static void removeMockUsableSpaceFS() { nodeEnvironment.close(); } - @After - public void cleanupThreadPool() { - testThreadPool.scheduledTasks.clear(); - } - static class TestCapturingThreadPool extends TestThreadPool { final List> scheduledTasks = new ArrayList<>(); @@ -319,8 +321,6 @@ public void testDiskSpaceMonitorStartsAsDisabled() throws Exception { ) ); } - aFileStore.throwIoException = false; - bFileStore.throwIoException = false; } public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Exception { @@ -406,8 +406,6 @@ public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Excep } }); } - aFileStore.throwIoException = false; - bFileStore.throwIoException = false; } public void testAvailableDiskSpaceMonitorSettingsUpdate() throws Exception { @@ -516,12 +514,6 @@ public void testAvailableDiskSpaceMonitorSettingsUpdate() throws Exception { } }, 5, TimeUnit.SECONDS); } - if (setThreadPoolMergeSchedulerSetting) { - assertWarnings( - "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " - + "and will be removed in a future release. See the breaking changes documentation for the next major version." - ); - } } public void testAbortingOrRunningMergeTaskHoldsUpBudget() throws Exception { @@ -564,7 +556,7 @@ public void testAbortingOrRunningMergeTaskHoldsUpBudget() throws Exception { testDoneLatch.await(); return null; }).when(stallingMergeTask).abort(); - threadPoolMergeExecutorService.submitMergeTask(stallingMergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(stallingMergeTask)); // assert the merge task is holding up disk space budget expectedAvailableBudget.set(expectedAvailableBudget.get() - taskBudget); assertBusy( @@ -574,7 +566,7 @@ public void testAbortingOrRunningMergeTaskHoldsUpBudget() throws Exception { ThreadPoolMergeScheduler.MergeTask mergeTask = mock(ThreadPoolMergeScheduler.MergeTask.class); when(mergeTask.estimatedRemainingMergeSize()).thenReturn(randomLongBetween(0L, expectedAvailableBudget.get())); when(mergeTask.schedule()).thenReturn(RUN); - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); assertBusy(() -> { verify(mergeTask).schedule(); verify(mergeTask).run(); @@ -595,12 +587,6 @@ public void testAbortingOrRunningMergeTaskHoldsUpBudget() throws Exception { assertThat(threadPoolMergeExecutorService.allDone(), is(true)); }); } - if (setThreadPoolMergeSchedulerSetting) { - assertWarnings( - "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " - + "and will be removed in a future release. See the breaking changes documentation for the next major version." - ); - } } public void testBackloggedMergeTasksDoNotHoldUpBudget() throws Exception { @@ -654,7 +640,7 @@ && randomBoolean()) { testDoneLatch.await(); return null; }).when(mergeTask).abort(); - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); if (mergeTask.schedule() == RUN) { runningMergeTasks.add(mergeTask); } else { @@ -679,7 +665,7 @@ && randomBoolean()) { return RUN; } }).when(mergeTask).schedule(); - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); backloggingMergeTasksScheduleCountMap.put(mergeTask, 1); } int checkRounds = randomIntBetween(1, 10); @@ -712,7 +698,7 @@ && randomBoolean()) { long taskBudget = randomLongBetween(1L, backloggedMergeTaskDiskSpaceBudget); when(mergeTask.estimatedRemainingMergeSize()).thenReturn(taskBudget); when(mergeTask.schedule()).thenReturn(RUN); - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); assertBusy(() -> { verify(mergeTask).schedule(); verify(mergeTask).run(); @@ -739,12 +725,6 @@ && randomBoolean()) { assertThat(threadPoolMergeExecutorService.allDone(), is(true)); }); } - if (setThreadPoolMergeSchedulerSetting) { - assertWarnings( - "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " - + "and will be removed in a future release. See the breaking changes documentation for the next major version." - ); - } } public void testUnavailableBudgetBlocksNewMergeTasksFromStartingExecution() throws Exception { @@ -823,7 +803,7 @@ public void testUnavailableBudgetBlocksNewMergeTasksFromStartingExecution() thro runningOrAbortingMergeTasksList.add(mergeTask); latchesBlockingMergeTasksList.add(blockMergeTaskLatch); } - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); } // currently running (or aborting) merge tasks have consumed some of the available budget while (runningOrAbortingMergeTasksList.isEmpty() == false) { @@ -855,8 +835,8 @@ public void testUnavailableBudgetBlocksNewMergeTasksFromStartingExecution() thro // merge task 2 can run because it is under budget when(mergeTask2.estimatedRemainingMergeSize()).thenReturn(underBudget); } - threadPoolMergeExecutorService.submitMergeTask(mergeTask1); - threadPoolMergeExecutorService.submitMergeTask(mergeTask2); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask1)); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask2)); assertBusy(() -> { if (task1Runs) { verify(mergeTask1).schedule(); @@ -890,12 +870,6 @@ public void testUnavailableBudgetBlocksNewMergeTasksFromStartingExecution() thro bFileStore.usableSpace = Long.MAX_VALUE; assertBusy(() -> assertThat(threadPoolMergeExecutorService.allDone(), is(true))); } - if (setThreadPoolMergeSchedulerSetting) { - assertWarnings( - "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " - + "and will be removed in a future release. See the breaking changes documentation for the next major version." - ); - } } public void testEnqueuedMergeTasksAreUnblockedWhenEstimatedMergeSizeChanges() throws Exception { @@ -990,12 +964,6 @@ public void testEnqueuedMergeTasksAreUnblockedWhenEstimatedMergeSizeChanges() th } }); } - if (setThreadPoolMergeSchedulerSetting) { - assertWarnings( - "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " - + "and will be removed in a future release. See the breaking changes documentation for the next major version." - ); - } } public void testMergeTasksAreUnblockedWhenMoreDiskSpaceBecomesAvailable() throws Exception { @@ -1058,7 +1026,7 @@ public void testMergeTasksAreUnblockedWhenMoreDiskSpaceBecomesAvailable() throws testDoneLatch.await(); return null; }).when(mergeTask).abort(); - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); if (mergeTask.schedule() == RUN) { runningMergeTasks.add(mergeTask); } else { @@ -1083,7 +1051,7 @@ public void testMergeTasksAreUnblockedWhenMoreDiskSpaceBecomesAvailable() throws when(mergeTask.estimatedRemainingMergeSize()).thenReturn(taskBudget); Schedule schedule = randomFrom(RUN, ABORT); when(mergeTask.schedule()).thenReturn(schedule); - threadPoolMergeExecutorService.submitMergeTask(mergeTask); + assertTrue(threadPoolMergeExecutorService.submitMergeTask(mergeTask)); if (schedule == RUN) { overBudgetTasksToRunList.add(mergeTask); } else { @@ -1150,11 +1118,5 @@ public void testMergeTasksAreUnblockedWhenMoreDiskSpaceBecomesAvailable() throws ); }); } - if (setThreadPoolMergeSchedulerSetting) { - assertWarnings( - "[indices.merge.scheduler.use_thread_pool] setting was deprecated in Elasticsearch " - + "and will be removed in a future release. See the breaking changes documentation for the next major version." - ); - } } }