[test] Fix IndexShardTests#testScheduledRefresh #110312

arteam · 2024-06-30T18:06:48Z

After we flushed the shard, we only make sure that the refresh call is propagated to the shard engine, but we can't be sure that the call is actually ends up in a shard refresh. The call in InternalEngine#refresh can return false if we couldn't acquire the lock on ElasticsearchDirectoryReader, because it's already being refreshed.

We can wrap the call in assertBusy to retry it in order to make sure that the shard eventually gets refreshed.

Resolves #101008

After we flushed the shard, we only make sure that the refresh call is propagated to the shard engine, but we can't be sure that the call is actually ends up in a shard refresh. The call in `InternalEngine#refresh` can return `false` if we couldn't acquire the lock on `ElasticsearchDirectoryReader`, because it's already being refreshed. We can wrap the call in `assertBusy` to retry it in order to make sure that the shard eventually gets refreshed. Resolves #101008

…fresh

elasticsearchmachine · 2024-07-01T06:20:16Z

Pinging @elastic/es-distributed (Team:Distributed)

fcofdez · 2024-07-02T15:39:50Z

The call in InternalEngine#refresh can return false if we couldn't acquire the lock on ElasticsearchDirectoryReader, because it's already being refreshed.

But this test concerns the IndexShard alone, meaning that there shouldn't be any background threads trying to refresh the shard (i.e. scheduled refreshes). Could you elaborate on which thread is calling refresh concurrently?

kingherc · 2024-07-02T16:04:35Z

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

@@ -3927,9 +3926,11 @@ public void testScheduledRefresh() throws Exception {
 assertTrue(primary.searchIdleTime() >= TimeValue.ZERO.millis());
 primary.flushOnIdle(0);
 logger.info("--> scheduledRefresh(future5)");


I have some questions related to the test (and not specifically to this line) that I don't understand:

Why is assertFalse(future4.actionGet());? The comment before future4 says while shard is search active and ensure scheduleRefresh(...) makes documen visible:. So I am not sure why the scheduled refresh returns false? I think that the shard was search idle, and that's why the scheduled refresh is false. Right? We should probably correct that comment.

Is the primary.flushOnIdle(0); actually flushing the shard? Doesn't flush also refresh? So how come assertTrue(future5.actionGet());? I would expect that since the shard was refreshed during the flush, that the scheduled refresh of future5 would return false.

Until I understand the test, I'm not sure I can follow the proposed solution. Any answers would be very helpful to me!

I think that the shard was search idle, and that's why the scheduled refresh is false. Right? We should probably correct that comment.

Yes, that's the reason why it returns false in this case, we should update the comment 👍.

Doesn't flush also refresh?

Notice that the refresh executed after a flush concerns the internal searcher (

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

Line 2225 in 6ae652f

refresh("version_table_flush", SearcherScope.INTERNAL, true);

) whereas the scheduled refresh concerns the external searcher (

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

Line 2020 in 6ae652f

ActionListener.completeWith(listener, () -> refresh(source, SearcherScope.EXTERNAL, false));

) hence the need for the explicit refresh.

If it's the flush thread racing against the scheduled refresh call what's causing this test failure, I'm not sure if the proposed solution will fix it, since it will return false always as the refresh will be a no-op.

Thanks @fcofdez for the clarification, I see!

If it's the flush thread racing against the scheduled refresh call what's causing this test failure, I'm not sure if the proposed solution will fix it, since it will return false always as the refresh will be a no-op.

Why is that? I think it actually may work. If there's a race with flush and refresh, the refresh may return false due to the race. But once the flush is executed, it makes active flag to be false, which should make the scheduledRefresh() finally refresh. So an assertBusy with scheduledRefresh should finally succeed. Right?

which should make the scheduledRefresh() finally refresh. So an assertBusy with scheduledRefresh should finally succeed. Right?

But if the refresh results in a no-op (there're no pending docs in the Lucene IndexWriter) the refresh will return false , right?

Isn't that assured by the previous indexDoc(primary, "_doc", "2", "{\"foo\" : \"bar\"}"); which has not been refreshed yet? I am thinking this is the reason the scheduled refresh actually returns true in the majority of the CI runs.

@kingherc I've added a check that the flush finishes before the refresh and a check that there are no pending refreshes.

At the same point, they do if (store.tryIncRef()) {, and the flush wins while the refresh does not win.

@kingherc but the store shouldn't be closed while the test is running 🤔 (both flush and refresh should be able to incRef the store ref unless the shard is closing)

The only possibility is that

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

Line 2046 in bdc65cb

refreshed = referenceManager.maybeRefresh();

returns false due to a concurrent refresh making this one a no-op.

@fcofdez , when I was quickly checking the code, I mistakenly thought tryIncRef does not have while-loop internally to retry, but it does, so my thinking was erroneous. Needs more in-depth investigation than my quick going-back-and-forth in the code finally :D

Still I believe it was not the intention of the test to let both the flush & the refresh to race with each other. However, I cannot say for sure why the racing results in refresh returning false. It would be good to understand why @arteam , plus whether there's some other refresh going on as @fcofdez suggests. @arteam were you able to reproduce it? Maybe we can add some more tracing to see if another refresh is going on.

Yeah, I think that when that test was written the flush call was executed synchronously... that needs a bit of digging :D

…fresh

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

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

This reverts commit a37a174.

kingherc · 2024-07-03T13:25:22Z

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

@@ -3927,9 +3926,11 @@ public void testScheduledRefresh() throws Exception {
 assertTrue(primary.searchIdleTime() >= TimeValue.ZERO.millis());
 primary.flushOnIdle(0);
 logger.info("--> scheduledRefresh(future5)");


@fcofdez , when I was quickly checking the code, I mistakenly thought tryIncRef does not have while-loop internally to retry, but it does, so my thinking was erroneous. Needs more in-depth investigation than my quick going-back-and-forth in the code finally :D

Still I believe it was not the intention of the test to let both the flush & the refresh to race with each other. However, I cannot say for sure why the racing results in refresh returning false. It would be good to understand why @arteam , plus whether there's some other refresh going on as @fcofdez suggests. @arteam were you able to reproduce it? Maybe we can add some more tracing to see if another refresh is going on.

kingherc · 2024-07-03T13:26:31Z

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

 logger.info("--> scheduledRefresh(future5)");
+ ensureNoPendingScheduledRefresh();


If there's some spurious refresh being run, we cannot be sure it is at this exact spot.

Maybe a better approach would be to get the number of external refreshes before the future5, and asserting that after the scheduled refresh, it's incremented by 1.

arteam added :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >test Issues or PRs that are addressing/adding tests labels Jun 30, 2024

elasticsearchmachine added the v8.15.0 label Jun 30, 2024

arteam added 2 commits June 30, 2024 22:24

Merge remote-tracking branch 'origin/main' into fix-test-scheduled-re…

9ed2634

…fresh

Run checkstyle

45e9ddb

arteam marked this pull request as ready for review July 1, 2024 06:19

arteam requested a review from fcofdez July 1, 2024 06:20

elasticsearchmachine added the Team:Distributed Meta label for distributed team label Jul 1, 2024

arteam requested review from kingherc and tlrx July 1, 2024 11:02

kingherc reviewed Jul 2, 2024

View reviewed changes

arteam added 2 commits July 3, 2024 11:05

Merge remote-tracking branch 'origin/main' into fix-test-scheduled-re…

d3781ab

…fresh

Add ability to distinguish whether we called refresh

a37a174

kingherc reviewed Jul 3, 2024

View reviewed changes

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java Outdated Show resolved Hide resolved

Revert "Add ability to distinguish whether we called refresh"

c57ce1a

This reverts commit a37a174.

arteam requested a review from kingherc July 3, 2024 10:25

arteam added 2 commits July 3, 2024 14:28

Make sure the flush completes and there are no pending refresh

5afadce

Use Phaser

26be0a1

arteam requested review from kingherc and removed request for kingherc July 3, 2024 12:34

Run spotless

1921630

kingherc reviewed Jul 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Fix IndexShardTests#testScheduledRefresh #110312

[test] Fix IndexShardTests#testScheduledRefresh #110312

arteam commented Jun 30, 2024

elasticsearchmachine commented Jul 1, 2024

fcofdez commented Jul 2, 2024

kingherc Jul 2, 2024

fcofdez Jul 2, 2024

kingherc Jul 2, 2024

fcofdez Jul 2, 2024

kingherc Jul 2, 2024

arteam Jul 3, 2024

fcofdez Jul 3, 2024

fcofdez Jul 3, 2024

kingherc Jul 3, 2024

fcofdez Jul 3, 2024

kingherc Jul 3, 2024

kingherc Jul 3, 2024

		logger.info("--> scheduledRefresh(future5)");
		ensureNoPendingScheduledRefresh();

[test] Fix IndexShardTests#testScheduledRefresh #110312

Are you sure you want to change the base?

[test] Fix IndexShardTests#testScheduledRefresh #110312

Conversation

arteam commented Jun 30, 2024

elasticsearchmachine commented Jul 1, 2024

fcofdez commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment