From c4239ac34355beaddbb2d9e57a408caa8e5d2a54 Mon Sep 17 00:00:00 2001 From: tlrx Date: Fri, 11 Apr 2025 14:28:32 +0200 Subject: [PATCH 1/2] [CI] Fix IndexShardTests.testReentrantEngineReadLockAcquisitionInRefreshListener I suspect the test resets/closes the reference manager between the refresh and the retrieval of the segment generation after the refresh. By executing segmentGenerationAfterRefresh while holding the engine reset lock we make sure there are no concurrent engine resets meanwhile. In the future, we should also ensure that IndexShard.refresh() uses withEngine. Closes #126628 --- muted-tests.yml | 3 -- .../index/engine/InternalEngine.java | 28 +++++++++++++------ .../index/shard/IndexShardTests.java | 2 +- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index e80b6c565c6c3..f20574f0c054a 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -423,9 +423,6 @@ tests: - class: org.elasticsearch.xpack.esql.action.ForkIT method: testWithStatsSimple issue: https://github.com/elastic/elasticsearch/issues/126607 -- class: org.elasticsearch.index.shard.IndexShardTests - method: testReentrantEngineReadLockAcquisitionInRefreshListener - issue: https://github.com/elastic/elasticsearch/issues/126628 - class: org.elasticsearch.upgrades.SearchStatesIT method: testBWCSearchStates issue: https://github.com/elastic/elasticsearch/issues/126632 diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index 3bd404a3e38a0..df350f7bc5ed2 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -2063,6 +2063,7 @@ protected final RefreshResult refresh(String source, SearcherScope scope, boolea engineReadLock.lock(); try { referenceManager.maybeRefreshBlocking(); + segmentGeneration = segmentGenerationAfterRefresh(referenceManager, generationBeforeRefresh); refreshed = true; } finally { engineReadLock.unlock(); @@ -2071,20 +2072,14 @@ protected final RefreshResult refresh(String source, SearcherScope scope, boolea if (engineReadLock.tryLock()) { try { refreshed = referenceManager.maybeRefresh(); + if (refreshed) { + segmentGeneration = segmentGenerationAfterRefresh(referenceManager, generationBeforeRefresh); + } } finally { engineReadLock.unlock(); } } } - if (refreshed) { - final ElasticsearchDirectoryReader current = referenceManager.acquire(); - try { - // Just use the generation from the reader when https://github.com/apache/lucene/pull/12177 is included. - segmentGeneration = Math.max(current.getIndexCommit().getGeneration(), generationBeforeRefresh); - } finally { - referenceManager.release(current); - } - } } finally { store.decRef(); } @@ -2120,6 +2115,21 @@ protected final RefreshResult refresh(String source, SearcherScope scope, boolea return new RefreshResult(refreshed, primaryTerm, segmentGeneration); } + private long segmentGenerationAfterRefresh( + ReferenceManager referenceManager, + long generationBeforeRefresh + ) throws IOException { + assert store.hasReferences(); + assert engineConfig.getEngineResetLock().isReadLockedByCurrentThread() : "prevent concurrent engine resets"; + final ElasticsearchDirectoryReader current = referenceManager.acquire(); + try { + // Just use the generation from the reader when https://github.com/apache/lucene/pull/12177 is included. + return Math.max(current.getIndexCommit().getGeneration(), generationBeforeRefresh); + } finally { + referenceManager.release(current); + } + } + @Override public void writeIndexingBuffer() throws IOException { final long reclaimableVersionMapBytes = versionMap.reclaimableRefreshRamBytes(); diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java index 5c3addfce5332..48257bbe3fa21 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -5274,7 +5274,7 @@ public void beforeRefresh() throws IOException { refreshStarted.countDown(); safeAwait(getFromTranslogStarted); - // A this stage, getThread is blocked on the refresh held by the current thread + // A this stage, getThread is blocked on the refresh lock held by the current thread assertBusy(() -> assertThat(engineResetLock.getReadLockCount(), greaterThanOrEqualTo(2))); assertThat(getFromTranslogResult.isDone(), equalTo(false)); From 337f453ce64782c19b6dd17e0dd83400f65b4d61 Mon Sep 17 00:00:00 2001 From: tlrx Date: Fri, 11 Apr 2025 15:25:45 +0200 Subject: [PATCH 2/2] wrong merge --- muted-tests.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 0a667a84b719f..e14a738419007 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -399,9 +399,6 @@ tests: - class: org.elasticsearch.xpack.test.rest.XPackRestIT method: test {p0=transform/transforms_start_stop/Test start/stop only starts/stops specified transform} issue: https://github.com/elastic/elasticsearch/issues/126466 -- class: org.elasticsearch.xpack.security.cli.HttpCertificateCommandTests - method: testGenerateMultipleCertificateWithNewCA - issue: https://github.com/elastic/elasticsearch/issues/126471 - class: org.elasticsearch.xpack.downsample.ILMDownsampleDisruptionIT method: testILMDownsampleRollingRestart issue: https://github.com/elastic/elasticsearch/issues/126495