UpdateSettingsIT#testEngineGCDeletesSetting failure

[talevy]

test failing:

org.elasticsearch.indices.settings.UpdateSettingsIT testEngineGCDeletesSetting

#38254 introduced a new VersionConflictEngineException assertion that seems
to be dependent on cache invalidation timed by Thread.sleep.

exception:

java.lang.AssertionError: expected a class org.elasticsearch.index.engine.VersionConflictEngineException exception to be thrown

	at __randomizedtesting.SeedInfo.seed([103812063D7BAB96:96D53E0C3D9BE721]:0)
	at org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows(ElasticsearchAssertions.java:596)
	at org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows(ElasticsearchAssertions.java:543)
	at org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows(ElasticsearchAssertions.java:520)
	at org.elasticsearch.indices.settings.UpdateSettingsIT.testEngineGCDeletesSetting(UpdateSettingsIT.java:452)

was tough to get a stacktrace, but looks to fail in InternalEngine#planIndexingAsPrimary

with the exception:

[type][1]: version conflict, required seqNo [3], primary term [1]. but no document was found

ci link: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+7.0+internalClusterTest/472/console

reproduce:

./gradlew :server:integTest \
  -Dtests.seed=EAF25E026AF61CCF \
  -Dtests.class=org.elasticsearch.indices.settings.UpdateSettingsIT \
  -Dtests.method="testEngineGCDeletesSetting" \
  -Dtests.security.manager=true \
  -Dtests.locale=is-IS \
  -Dtests.timezone=Australia/Canberra \
  -Dcompiler.java=11 \
  -Druntime.java=8

This is easy to reproduce if one removes the Thread.sleep(300). Which means that
cache hadn't changed in the CI run in time for the assertion.

[elasticmachine]

Pinging @elastic/es-distributed

[talevy]

Thread.sleep(300); // wait for cache time to change TODO: this needs to be solved better. To be discussed.

The timing issue has a TODO, so it may be worth discussion instead of simply increasing the timeout?

[talevy]

@matriv thought about this one, this diff should resolve the problem

--- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java
+++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java
@@ -435,7 +435,7 @@ public class UpdateSettingsIT extends ESIntegTestCase {
         assertThat(getSettingsResponse.getSetting("test", "index.final"), nullValue());
     }
 
-    public void testEngineGCDeletesSetting() throws InterruptedException {
+    public void testEngineGCDeletesSetting() throws Exception {
         createIndex("test");
         client().prepareIndex("test", "type", "1").setSource("f", 1).get();
         DeleteResponse response = client().prepareDelete("test", "type", "1").get();
@@ -445,12 +445,10 @@ public class UpdateSettingsIT extends ESIntegTestCase {
         client().prepareIndex("test", "type", "1").setSource("f", 2).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm).get();
         client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder().put("index.gc_deletes", 0)).get();
 
-        response = client().prepareDelete("test", "type", "1").get();
-        seqNo = response.getSeqNo();
-        Thread.sleep(300); // wait for cache time to change TODO: this needs to be solved better. To be discussed.
+        DeleteResponse secondDeleteResponse = client().prepareDelete("test", "type", "1").get();
         // delete is should not be in cache
-        assertThrows(client().prepareIndex("test", "type", "1").setSource("f", 3).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm),
-            VersionConflictEngineException.class);
+        assertBusy(() -> assertThrows(client().prepareIndex("test", "type", "1").setSource("f", 3)
+                .setIfSeqNo(secondDeleteResponse.getSeqNo()).setIfPrimaryTerm(primaryTerm), VersionConflictEngineException.class));
 
     }

[matriv]

@talevy Thanks for that.
We had a discussion with @bleskes who helped me here (thank you!) and we came up with: #38942 .