Don't consider number of working nodes in coverage

Tring to figure out the right groups to send queries to based
on the number of nodes in the group has many potential issues
at times of topology changes.

Since we could the number of documents available in each group
by summing documents in working nodes, we do not need to also
separately consider the number of working nodes in the group for
correctness.

Since we use adaptive dispatching by default we also do not need
to consider it to avoid overloading groups with less resources
available but enough documents.

3 files changed, 22 insertions, 24 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/InvokerFactory.java b/container-search/src/main/java/com/yahoo/search/dispatch/InvokerFactory.java
index 1bcb640e3a5..1de274ce6cf 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/InvokerFactory.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/InvokerFactory.java
@@ -82,7 +82,7 @@ public abstract class InvokerFactory {
             if ( ! searchCluster.isPartialGroupCoverageSufficient(success) && !acceptIncompleteCoverage) {
                 return Optional.empty();
             }
-            if (invokers.size() == 0) {
+            if (invokers.isEmpty()) {
                 return Optional.of(createCoverageErrorInvoker(nodes, failed));
             }
         }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
index ce834b108db..159a42676ec 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
@@ -311,19 +311,17 @@ public class SearchCluster implements NodeManager<Node> {
         // With just one group sufficient coverage may not be the same as full coverage, as the
         // group will always be marked sufficient for use.
         updateSufficientCoverage(group, true);
-        boolean sufficientCoverage = isGroupCoverageSufficient(group.workingNodes(),
-                                                               group.getActiveDocuments(),
+        boolean sufficientCoverage = isGroupCoverageSufficient(group.getActiveDocuments(),
                                                                group.getActiveDocuments());
         trackGroupCoverageChanges(group, sufficientCoverage, group.getActiveDocuments());
     }
 
     private void pingIterationCompletedMultipleGroups() {
-        aggregateNodeValues();
+        orderedGroups().forEach(Group::aggregateNodeValues);
         long medianDocuments = medianDocumentsPerGroup();
         boolean anyGroupsSufficientCoverage = false;
         for (Group group : orderedGroups()) {
-            boolean sufficientCoverage = isGroupCoverageSufficient(group.workingNodes(),
-                                                                   group.getActiveDocuments(),
+            boolean sufficientCoverage = isGroupCoverageSufficient(group.getActiveDocuments(),
                                                                    medianDocuments);
             anyGroupsSufficientCoverage = anyGroupsSufficientCoverage || sufficientCoverage;
             updateSufficientCoverage(group, sufficientCoverage);
@@ -331,10 +329,6 @@ public class SearchCluster implements NodeManager<Node> {
         }
     }
 
-    private void aggregateNodeValues() {
-        orderedGroups().forEach(Group::aggregateNodeValues);
-    }
-
     private long medianDocumentsPerGroup() {
         if (orderedGroups().isEmpty()) return 0;
         var activeDocuments = orderedGroups().stream().map(Group::getActiveDocuments).collect(Collectors.toList());
@@ -356,23 +350,13 @@ public class SearchCluster implements NodeManager<Node> {
         }
     }
 
-    private boolean isGroupCoverageSufficient(int workingNodesInGroup, long activeDocuments, long medianDocuments) {
+    private boolean isGroupCoverageSufficient(long activeDocuments, long medianDocuments) {
         double documentCoverage = 100.0 * (double) activeDocuments / medianDocuments;
         if (medianDocuments > 0 && documentCoverage < dispatchConfig.minActivedocsPercentage())
             return false;
-
-        if ( ! isGroupNodeCoverageSufficient(workingNodesInGroup))
-            return false;
-
         return true;
     }
 
-    private boolean isGroupNodeCoverageSufficient(int workingNodesInGroup) {
-        int nodesAllowedDown = dispatchConfig.maxNodesDownPerGroup()
-                + (int) (((double) wantedGroupSize() * (100.0 - dispatchConfig.minGroupCoverage())) / 100.0);
-        return workingNodesInGroup + nodesAllowedDown >= wantedGroupSize();
-    }
-
     public boolean isGroupWellBalanced(OptionalInt groupId) {
         if (groupId.isEmpty()) return false;
         Group group = groups().get(groupId.getAsInt());
@@ -386,7 +370,7 @@ public class SearchCluster implements NodeManager<Node> {
         if (orderedGroups().size() == 1)
             return nodes.size() >= wantedGroupSize() - dispatchConfig.maxNodesDownPerGroup();
         long activeDocuments = nodes.stream().mapToLong(Node::getActiveDocuments).sum();
-        return isGroupCoverageSufficient(nodes.size(), activeDocuments, medianDocumentsPerGroup());
+        return isGroupCoverageSufficient(activeDocuments, medianDocumentsPerGroup());
     }
 
     private void trackGroupCoverageChanges(Group group, boolean fullCoverage, long medianDocuments) {
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterCoverageTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterCoverageTest.java
index 6338107d4b6..65e7173c4ee 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterCoverageTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterCoverageTest.java
@@ -74,10 +74,10 @@ public class SearchClusterCoverageTest {
 
     @Test
     public void three_groups_one_has_a_node_down() {
-        var tester =  new SearchClusterTester(3, 3);
+        var tester = new SearchClusterTester(3, 3);
 
         tester.setDocsPerNode(100, 0);
-        tester.setDocsPerNode(150, 1);
+        tester.setDocsPerNode(100, 1);
         tester.setDocsPerNode(100, 2);
         tester.setWorking(1, 1, false);
         tester.pingIterationCompleted();
@@ -86,4 +86,18 @@ public class SearchClusterCoverageTest {
         assertTrue(tester.group(2).hasSufficientCoverage());
     }
 
+    @Test
+    public void three_groups_one_has_a_node_down_but_remaining_has_enough_docs() {
+        var tester = new SearchClusterTester(3, 3);
+
+        tester.setDocsPerNode(100, 0);
+        tester.setDocsPerNode(150, 1);
+        tester.setDocsPerNode(100, 2);
+        tester.setWorking(1, 1, false);
+        tester.pingIterationCompleted();
+        assertTrue(tester.group(0).hasSufficientCoverage());
+        assertTrue("Sufficient documents on remaining two nodes", tester.group(1).hasSufficientCoverage());
+        assertTrue(tester.group(2).hasSufficientCoverage());
+    }
+
 }