Skip to content

Commit c4239ac

Browse files
committed
[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
1 parent dd1db50 commit c4239ac

File tree

3 files changed

+20
-13
lines changed

3 files changed

+20
-13
lines changed

muted-tests.yml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -423,9 +423,6 @@ tests:
423423
- class: org.elasticsearch.xpack.esql.action.ForkIT
424424
method: testWithStatsSimple
425425
issue: https://github.com/elastic/elasticsearch/issues/126607
426-
- class: org.elasticsearch.index.shard.IndexShardTests
427-
method: testReentrantEngineReadLockAcquisitionInRefreshListener
428-
issue: https://github.com/elastic/elasticsearch/issues/126628
429426
- class: org.elasticsearch.upgrades.SearchStatesIT
430427
method: testBWCSearchStates
431428
issue: https://github.com/elastic/elasticsearch/issues/126632

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2063,6 +2063,7 @@ protected final RefreshResult refresh(String source, SearcherScope scope, boolea
20632063
engineReadLock.lock();
20642064
try {
20652065
referenceManager.maybeRefreshBlocking();
2066+
segmentGeneration = segmentGenerationAfterRefresh(referenceManager, generationBeforeRefresh);
20662067
refreshed = true;
20672068
} finally {
20682069
engineReadLock.unlock();
@@ -2071,20 +2072,14 @@ protected final RefreshResult refresh(String source, SearcherScope scope, boolea
20712072
if (engineReadLock.tryLock()) {
20722073
try {
20732074
refreshed = referenceManager.maybeRefresh();
2075+
if (refreshed) {
2076+
segmentGeneration = segmentGenerationAfterRefresh(referenceManager, generationBeforeRefresh);
2077+
}
20742078
} finally {
20752079
engineReadLock.unlock();
20762080
}
20772081
}
20782082
}
2079-
if (refreshed) {
2080-
final ElasticsearchDirectoryReader current = referenceManager.acquire();
2081-
try {
2082-
// Just use the generation from the reader when https://github.com/apache/lucene/pull/12177 is included.
2083-
segmentGeneration = Math.max(current.getIndexCommit().getGeneration(), generationBeforeRefresh);
2084-
} finally {
2085-
referenceManager.release(current);
2086-
}
2087-
}
20882083
} finally {
20892084
store.decRef();
20902085
}
@@ -2120,6 +2115,21 @@ protected final RefreshResult refresh(String source, SearcherScope scope, boolea
21202115
return new RefreshResult(refreshed, primaryTerm, segmentGeneration);
21212116
}
21222117

2118+
private long segmentGenerationAfterRefresh(
2119+
ReferenceManager<ElasticsearchDirectoryReader> referenceManager,
2120+
long generationBeforeRefresh
2121+
) throws IOException {
2122+
assert store.hasReferences();
2123+
assert engineConfig.getEngineResetLock().isReadLockedByCurrentThread() : "prevent concurrent engine resets";
2124+
final ElasticsearchDirectoryReader current = referenceManager.acquire();
2125+
try {
2126+
// Just use the generation from the reader when https://github.com/apache/lucene/pull/12177 is included.
2127+
return Math.max(current.getIndexCommit().getGeneration(), generationBeforeRefresh);
2128+
} finally {
2129+
referenceManager.release(current);
2130+
}
2131+
}
2132+
21232133
@Override
21242134
public void writeIndexingBuffer() throws IOException {
21252135
final long reclaimableVersionMapBytes = versionMap.reclaimableRefreshRamBytes();

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5274,7 +5274,7 @@ public void beforeRefresh() throws IOException {
52745274
refreshStarted.countDown();
52755275
safeAwait(getFromTranslogStarted);
52765276

5277-
// A this stage, getThread is blocked on the refresh held by the current thread
5277+
// A this stage, getThread is blocked on the refresh lock held by the current thread
52785278
assertBusy(() -> assertThat(engineResetLock.getReadLockCount(), greaterThanOrEqualTo(2)));
52795279
assertThat(getFromTranslogResult.isDone(), equalTo(false));
52805280

0 commit comments

Comments
 (0)