From 7fbdcb5e00d6861d099394cac50c08a8abb7b741 Mon Sep 17 00:00:00 2001
From: Armin Braun <me@obrown.io>
Date: Mon, 23 Nov 2020 12:30:18 +0100
Subject: [PATCH] Fix
 SearchableSnapshotsIntegTests.testCreateAndRestoreSearchableSnapshot (#65343)
 (#65351)

The recovery stats assertions in this test ran without any waiting for
the recoveries to actually finish. The fact that they ran after the concurrent
searches checks generally meant that they would pass (because of searches warming caches
+ general relative slowness of searches) but there is no hard guarantees this will work
reliably as the pre-fetch threads which will update the recovery state might still be slow
to do so randomly, causing the assertions to trip.

closes #65302
---
 .../SearchableSnapshotsIntegTests.java        | 47 ++++++++++---------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java
index 821f1111e9f..67d5f6581ac 100644
--- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java
+++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java
@@ -64,6 +64,7 @@ import java.util.Set;
 import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 import java.util.stream.Stream;
@@ -700,32 +701,34 @@ public class SearchableSnapshotsIntegTests extends BaseSearchableSnapshotsIntegT
         }
     }
 
-    private void assertRecoveryStats(String indexName, boolean preWarmEnabled) {
+    private void assertRecoveryStats(String indexName, boolean preWarmEnabled) throws Exception {
         int shardCount = getNumShards(indexName).totalNumShards;
-        final RecoveryResponse recoveryResponse = client().admin().indices().prepareRecoveries(indexName).get();
-        assertThat(recoveryResponse.toString(), recoveryResponse.shardRecoveryStates().get(indexName).size(), equalTo(shardCount));
+        assertBusy(() -> {
+            final RecoveryResponse recoveryResponse = client().admin().indices().prepareRecoveries(indexName).get();
+            assertThat(recoveryResponse.toString(), recoveryResponse.shardRecoveryStates().get(indexName).size(), equalTo(shardCount));
 
-        for (List<RecoveryState> recoveryStates : recoveryResponse.shardRecoveryStates().values()) {
-            for (RecoveryState recoveryState : recoveryStates) {
-                ByteSizeValue cacheSize = getCacheSizeForNode(recoveryState.getTargetNode().getName());
-                boolean unboundedCache = cacheSize.equals(new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES));
-                RecoveryState.Index index = recoveryState.getIndex();
-                assertThat(
-                    Strings.toString(recoveryState, true, true),
-                    index.recoveredFileCount(),
-                    preWarmEnabled && unboundedCache ? equalTo(index.totalRecoverFiles()) : greaterThanOrEqualTo(0)
-                );
+            for (List<RecoveryState> recoveryStates : recoveryResponse.shardRecoveryStates().values()) {
+                for (RecoveryState recoveryState : recoveryStates) {
+                    ByteSizeValue cacheSize = getCacheSizeForNode(recoveryState.getTargetNode().getName());
+                    boolean unboundedCache = cacheSize.equals(new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES));
+                    RecoveryState.Index index = recoveryState.getIndex();
+                    assertThat(
+                        Strings.toString(recoveryState, true, true),
+                        index.recoveredFileCount(),
+                        preWarmEnabled && unboundedCache ? equalTo(index.totalRecoverFiles()) : greaterThanOrEqualTo(0)
+                    );
 
-                // Since the cache size is variable, the pre-warm phase might fail as some of the files can be evicted
-                // while a part is pre-fetched, in that case the recovery state stage is left as FINALIZE.
-                assertThat(
-                    recoveryState.getStage(),
-                    unboundedCache
-                        ? equalTo(RecoveryState.Stage.DONE)
-                        : anyOf(equalTo(RecoveryState.Stage.DONE), equalTo(RecoveryState.Stage.FINALIZE))
-                );
+                    // Since the cache size is variable, the pre-warm phase might fail as some of the files can be evicted
+                    // while a part is pre-fetched, in that case the recovery state stage is left as FINALIZE.
+                    assertThat(
+                        recoveryState.getStage(),
+                        unboundedCache
+                            ? equalTo(RecoveryState.Stage.DONE)
+                            : anyOf(equalTo(RecoveryState.Stage.DONE), equalTo(RecoveryState.Stage.FINALIZE))
+                    );
+                }
             }
-        }
+        }, 30L, TimeUnit.SECONDS);
     }
 
     private void assertSearchableSnapshotStats(String indexName, boolean cacheEnabled, List<String> nonCachedExtensions) {