From 47a3055e8aacd15181ae294b91617aa2ac5c4b90 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Wed, 25 Jun 2025 20:00:31 +0300 Subject: [PATCH 1/3] Fix 129149 --- muted-tests.yml | 3 -- ...oolMergeExecutorServiceDiskSpaceTests.java | 42 ++++++++----------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 72de2e65e2783..4354a947a802d 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -473,9 +473,6 @@ tests: - class: org.elasticsearch.packaging.test.DockerTests method: test081SymlinksAreFollowedWithEnvironmentVariableFiles issue: https://github.com/elastic/elasticsearch/issues/128867 -- class: org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests - method: testAvailableDiskSpaceMonitorWhenFileSystemStatErrors - issue: https://github.com/elastic/elasticsearch/issues/129149 - class: org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests method: testUnavailableBudgetBlocksNewMergeTasksFromStartingExecution issue: https://github.com/elastic/elasticsearch/issues/129148 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 5b97878b18450..61c78763389fa 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java @@ -324,10 +324,19 @@ public void testDiskSpaceMonitorStartsAsDisabled() throws Exception { } public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Exception { - aFileStore.usableSpace = randomLongBetween(1L, 100L); - aFileStore.totalSpace = randomLongBetween(1L, 100L); - bFileStore.usableSpace = randomLongBetween(1L, 100L); - bFileStore.totalSpace = randomLongBetween(1L, 100L); + long aUsableSpace; + long bUsableSpace; + do { + aFileStore.usableSpace = randomLongBetween(1L, 1000L); + aFileStore.totalSpace = randomLongBetween(1L, 1000L); + bFileStore.usableSpace = randomLongBetween(1L, 1000L); + bFileStore.totalSpace = randomLongBetween(1L, 1000L); + // the default 5% (same as flood stage level) + aUsableSpace = Math.max(aFileStore.usableSpace - aFileStore.totalSpace / 20, 0L); + bUsableSpace = Math.max(bFileStore.usableSpace - bFileStore.totalSpace / 20, 0L); + } while (aUsableSpace == bUsableSpace); // they must be different in order to distinguish the available disk space updates + long finalBUsableSpace = bUsableSpace; + long finalAUsableSpace = aUsableSpace; boolean aErrorsFirst = randomBoolean(); if (aErrorsFirst) { // the "a" file system will error when collecting stats @@ -355,18 +364,10 @@ public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Excep assertThat(availableDiskSpaceUpdates.size(), is(1)); if (aErrorsFirst) { // uses the stats from "b" - assertThat( - availableDiskSpaceUpdates.getLast().getBytes(), - // the default 5% (same as flood stage level) - is(Math.max(bFileStore.usableSpace - bFileStore.totalSpace / 20, 0L)) - ); + assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalBUsableSpace)); } else { // uses the stats from "a" - assertThat( - availableDiskSpaceUpdates.getLast().getBytes(), - // the default 5% (same as flood stage level) - is(Math.max(aFileStore.usableSpace - aFileStore.totalSpace / 20, 0L)) - ); + assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalAUsableSpace)); } } }); @@ -393,21 +394,14 @@ public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Excep } assertBusy(() -> { synchronized (availableDiskSpaceUpdates) { + // the updates are different values assertThat(availableDiskSpaceUpdates.size(), is(3)); if (aErrorsFirst) { // uses the stats from "a" - assertThat( - availableDiskSpaceUpdates.getLast().getBytes(), - // the default 5% (same as flood stage level) - is(Math.max(aFileStore.usableSpace - aFileStore.totalSpace / 20, 0L)) - ); + assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalAUsableSpace)); } else { // uses the stats from "b" - assertThat( - availableDiskSpaceUpdates.getLast().getBytes(), - // the default 5% (same as flood stage level) - is(Math.max(bFileStore.usableSpace - bFileStore.totalSpace / 20, 0L)) - ); + assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalBUsableSpace)); } } }); From 1539d43f355954cc5e6266bff76138b4da38096f Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 26 Jun 2025 11:29:52 +0300 Subject: [PATCH 2/3] Fixes race in test between merge task scheduling and scheduler close --- muted-tests.yml | 3 --- .../index/engine/ThreadPoolMergeSchedulerTests.java | 5 +++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 107d614d19720..ba43ac7087cfd 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -281,9 +281,6 @@ tests: - class: org.elasticsearch.search.basic.SearchWithRandomDisconnectsIT method: testSearchWithRandomDisconnects issue: https://github.com/elastic/elasticsearch/issues/122707 -- class: org.elasticsearch.index.engine.ThreadPoolMergeSchedulerTests - method: testSchedulerCloseWaitsForRunningMerge - issue: https://github.com/elastic/elasticsearch/issues/125236 - class: org.elasticsearch.packaging.test.DockerTests method: test020PluginsListWithNoPlugins issue: https://github.com/elastic/elasticsearch/issues/126232 diff --git a/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java b/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java index d88f7c67b0bbd..bea7697fd51c6 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeSchedulerTests.java @@ -612,11 +612,12 @@ public void testSchedulerCloseWaitsForRunningMerge() throws Exception { fail(e); } }); + // test expects that there definitely is a running merge before closing the merge scheduler + mergeRunningLatch.await(); + // closes the merge scheduler t.start(); try { assertTrue(t.isAlive()); - // wait for the merge to actually run - mergeRunningLatch.await(); // ensure the merge scheduler is effectively "closed" assertBusy(() -> { MergeSource mergeSource2 = mock(MergeSource.class); From a70c16bbdb9d948a4d53edb1a819fce4185282f2 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 26 Jun 2025 12:11:57 +0300 Subject: [PATCH 3/3] Revert "Fix 129149" This reverts commit 47a3055e8aacd15181ae294b91617aa2ac5c4b90. --- muted-tests.yml | 3 ++ ...oolMergeExecutorServiceDiskSpaceTests.java | 42 +++++++++++-------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index ba43ac7087cfd..8fb109c2552ad 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -470,6 +470,9 @@ tests: - class: org.elasticsearch.packaging.test.DockerTests method: test081SymlinksAreFollowedWithEnvironmentVariableFiles issue: https://github.com/elastic/elasticsearch/issues/128867 +- class: org.elasticsearch.index.engine.ThreadPoolMergeExecutorServiceDiskSpaceTests + method: testAvailableDiskSpaceMonitorWhenFileSystemStatErrors + issue: https://github.com/elastic/elasticsearch/issues/129149 - class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeForkIT method: test {lookup-join.EnrichLookupStatsBug ASYNC} issue: https://github.com/elastic/elasticsearch/issues/129228 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..4653f8210cda3 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceDiskSpaceTests.java @@ -324,19 +324,10 @@ public void testDiskSpaceMonitorStartsAsDisabled() throws Exception { } public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Exception { - long aUsableSpace; - long bUsableSpace; - do { - aFileStore.usableSpace = randomLongBetween(1L, 1000L); - aFileStore.totalSpace = randomLongBetween(1L, 1000L); - bFileStore.usableSpace = randomLongBetween(1L, 1000L); - bFileStore.totalSpace = randomLongBetween(1L, 1000L); - // the default 5% (same as flood stage level) - aUsableSpace = Math.max(aFileStore.usableSpace - aFileStore.totalSpace / 20, 0L); - bUsableSpace = Math.max(bFileStore.usableSpace - bFileStore.totalSpace / 20, 0L); - } while (aUsableSpace == bUsableSpace); // they must be different in order to distinguish the available disk space updates - long finalBUsableSpace = bUsableSpace; - long finalAUsableSpace = aUsableSpace; + aFileStore.usableSpace = randomLongBetween(1L, 100L); + aFileStore.totalSpace = randomLongBetween(1L, 100L); + bFileStore.usableSpace = randomLongBetween(1L, 100L); + bFileStore.totalSpace = randomLongBetween(1L, 100L); boolean aErrorsFirst = randomBoolean(); if (aErrorsFirst) { // the "a" file system will error when collecting stats @@ -364,10 +355,18 @@ public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Excep assertThat(availableDiskSpaceUpdates.size(), is(1)); if (aErrorsFirst) { // uses the stats from "b" - assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalBUsableSpace)); + assertThat( + availableDiskSpaceUpdates.getLast().getBytes(), + // the default 5% (same as flood stage level) + is(Math.max(bFileStore.usableSpace - bFileStore.totalSpace / 20, 0L)) + ); } else { // uses the stats from "a" - assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalAUsableSpace)); + assertThat( + availableDiskSpaceUpdates.getLast().getBytes(), + // the default 5% (same as flood stage level) + is(Math.max(aFileStore.usableSpace - aFileStore.totalSpace / 20, 0L)) + ); } } }); @@ -394,14 +393,21 @@ public void testAvailableDiskSpaceMonitorWhenFileSystemStatErrors() throws Excep } assertBusy(() -> { synchronized (availableDiskSpaceUpdates) { - // the updates are different values assertThat(availableDiskSpaceUpdates.size(), is(3)); if (aErrorsFirst) { // uses the stats from "a" - assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalAUsableSpace)); + assertThat( + availableDiskSpaceUpdates.getLast().getBytes(), + // the default 5% (same as flood stage level) + is(Math.max(aFileStore.usableSpace - aFileStore.totalSpace / 20, 0L)) + ); } else { // uses the stats from "b" - assertThat(availableDiskSpaceUpdates.getLast().getBytes(), is(finalBUsableSpace)); + assertThat( + availableDiskSpaceUpdates.getLast().getBytes(), + // the default 5% (same as flood stage level) + is(Math.max(bFileStore.usableSpace - bFileStore.totalSpace / 20, 0L)) + ); } } });