Merge pull request #17807 from vespa-engine/revert-17798-bratseth/dispatch-group-logic

Revert "Don't consider number of working nodes in coverage"

3 files changed, 24 insertions, 22 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 1de274ce6cf..1bcb640e3a5 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.isEmpty()) {
+            if (invokers.size() == 0) {
                 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 159a42676ec..ce834b108db 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,17 +311,19 @@ 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.getActiveDocuments(),
+        boolean sufficientCoverage = isGroupCoverageSufficient(group.workingNodes(),
+                                                               group.getActiveDocuments(),
                                                                group.getActiveDocuments());
         trackGroupCoverageChanges(group, sufficientCoverage, group.getActiveDocuments());
     }
 
     private void pingIterationCompletedMultipleGroups() {
-        orderedGroups().forEach(Group::aggregateNodeValues);
+        aggregateNodeValues();
         long medianDocuments = medianDocumentsPerGroup();
         boolean anyGroupsSufficientCoverage = false;
         for (Group group : orderedGroups()) {
-            boolean sufficientCoverage = isGroupCoverageSufficient(group.getActiveDocuments(),
+            boolean sufficientCoverage = isGroupCoverageSufficient(group.workingNodes(),
+                                                                   group.getActiveDocuments(),
                                                                    medianDocuments);
             anyGroupsSufficientCoverage = anyGroupsSufficientCoverage || sufficientCoverage;
             updateSufficientCoverage(group, sufficientCoverage);
@@ -329,6 +331,10 @@ 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());
@@ -350,13 +356,23 @@ public class SearchCluster implements NodeManager<Node> {
         }
     }
 
-    private boolean isGroupCoverageSufficient(long activeDocuments, long medianDocuments) {
+    private boolean isGroupCoverageSufficient(int workingNodesInGroup, 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());
@@ -370,7 +386,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(activeDocuments, medianDocumentsPerGroup());
+        return isGroupCoverageSufficient(nodes.size(), 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 65e7173c4ee..6338107d4b6 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,21 +74,7 @@ public class SearchClusterCoverageTest {
 
     @Test
     public void three_groups_one_has_a_node_down() {
-        var tester = new SearchClusterTester(3, 3);
-
-        tester.setDocsPerNode(100, 0);
-        tester.setDocsPerNode(100, 1);
-        tester.setDocsPerNode(100, 2);
-        tester.setWorking(1, 1, false);
-        tester.pingIterationCompleted();
-        assertTrue(tester.group(0).hasSufficientCoverage());
-        assertFalse(tester.group(1).hasSufficientCoverage());
-        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);
+        var tester =  new SearchClusterTester(3, 3);
 
         tester.setDocsPerNode(100, 0);
         tester.setDocsPerNode(150, 1);
@@ -96,7 +82,7 @@ public class SearchClusterCoverageTest {
         tester.setWorking(1, 1, false);
         tester.pingIterationCompleted();
         assertTrue(tester.group(0).hasSufficientCoverage());
-        assertTrue("Sufficient documents on remaining two nodes", tester.group(1).hasSufficientCoverage());
+        assertFalse(tester.group(1).hasSufficientCoverage());
         assertTrue(tester.group(2).hasSufficientCoverage());
     }