Fix SearchReplicaSelectionIT failures

This PR makes sure MockSearchService collects ARS statistics. Before, if we randomly chose to use MockSearchService then ARS information would be missing and the test would fail. Also makes the following fixes: * Removes a test workaround for the bug elastic#71022, which is now fixed. * Handle the case where nodes have same rank, to prevent random failures.
jtibshirani · Apr 8, 2021 · c5faa94 · c5faa94
1 parent a613f03
commit c5faa94
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 19 deletions.
diff --git a/...c/internalClusterTest/java/org/elasticsearch/search/routing/SearchReplicaSelectionIT.java b/...c/internalClusterTest/java/org/elasticsearch/search/routing/SearchReplicaSelectionIT.java
@@ -17,7 +17,6 @@
 import org.elasticsearch.cluster.routing.OperationRouting;
 import org.elasticsearch.common.collect.ImmutableOpenMap;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.node.ResponseCollectorService.ComputedNodeStats;
 import org.elasticsearch.test.ESIntegTestCase;
 
 import java.util.HashSet;
@@ -28,6 +27,7 @@
 import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS;
 import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 
 @ESIntegTestCase.ClusterScope(numClientNodes = 1, numDataNodes = 3)
 public class SearchReplicaSelectionIT extends ESIntegTestCase {
@@ -38,7 +38,6 @@ public Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
             .put(OperationRouting.USE_ADAPTIVE_REPLICA_SELECTION_SETTING.getKey(), true).build();
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/70621")
     public void testNodeSelection() {
         // We grab a client directly to avoid using a randomizing client that might set a search preference.
         Client client = internalCluster().coordOnlyNodeClient();
@@ -69,7 +68,7 @@ public void testNodeSelection() {
 
         assertEquals(3, nodeIds.size());
 
-        // Now after more searches, we should select the node with the best ARS rank.
+        // Now after more searches, we should select a node with the lowest ARS rank.
         for (int i = 0; i < 5; i++) {
             client.prepareSearch().setQuery(matchAllQuery()).get();
         }
@@ -86,17 +85,13 @@ public void testNodeSelection() {
         assertNotNull(nodeStats);
         assertEquals(3, nodeStats.getAdaptiveSelectionStats().getComputedStats().size());
 
-        String bestNodeId = null;
-        double bestRank = Double.POSITIVE_INFINITY;
-        for (Map.Entry<String, ComputedNodeStats> entry : nodeStats.getAdaptiveSelectionStats().getComputedStats().entrySet()) {
-            double rank = entry.getValue().rank(1L);
-            if (rank < bestRank) {
-                bestNodeId = entry.getKey();
-                bestRank = rank;
-            }
-        }
-
         searchResponse = client.prepareSearch().setQuery(matchAllQuery()).get();
-        assertEquals(bestNodeId, searchResponse.getHits().getAt(0).getShard().getNodeId());
+        String selectedNodeId = searchResponse.getHits().getAt(0).getShard().getNodeId();
+        double selectedRank = nodeStats.getAdaptiveSelectionStats().getRanks().get(selectedNodeId);
+
+        for (Map.Entry<String, Double> entry : nodeStats.getAdaptiveSelectionStats().getRanks().entrySet()) {
+            double rank = entry.getValue();
+            assertThat(rank, greaterThanOrEqualTo(selectedRank));
+        }
     }
 }
diff --git a/test/framework/src/main/java/org/elasticsearch/node/MockNode.java b/test/framework/src/main/java/org/elasticsearch/node/MockNode.java
@@ -124,7 +124,7 @@ protected SearchService newSearchService(ClusterService clusterService, IndicesS
                 responseCollectorService, circuitBreakerService);
         }
         return new MockSearchService(clusterService, indicesService, threadPool, scriptService,
-            bigArrays, fetchPhase, circuitBreakerService);
+            bigArrays, fetchPhase, responseCollectorService, circuitBreakerService);
     }
 
     @Override

diff --git a/test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java b/test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java
@@ -14,6 +14,7 @@
 import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.indices.breaker.CircuitBreakerService;
 import org.elasticsearch.node.MockNode;
+import org.elasticsearch.node.ResponseCollectorService;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.script.ScriptService;
 import org.elasticsearch.search.fetch.FetchPhase;
@@ -68,10 +69,11 @@ static void removeActiveContext(ReaderContext context) {
         ACTIVE_SEARCH_CONTEXTS.remove(context);
     }
 
-    public MockSearchService(ClusterService clusterService,
-            IndicesService indicesService, ThreadPool threadPool, ScriptService scriptService,
-            BigArrays bigArrays, FetchPhase fetchPhase, CircuitBreakerService circuitBreakerService) {
-        super(clusterService, indicesService, threadPool, scriptService, bigArrays, fetchPhase, null, circuitBreakerService);
+    public MockSearchService(ClusterService clusterService, IndicesService indicesService, ThreadPool threadPool,
+                             ScriptService scriptService, BigArrays bigArrays, FetchPhase fetchPhase,
+                             ResponseCollectorService responseCollectorService, CircuitBreakerService circuitBreakerService) {
+        super(clusterService, indicesService, threadPool, scriptService, bigArrays, fetchPhase, responseCollectorService,
+            circuitBreakerService);
     }
 
     @Override