Merge pull request #18511 from vespa-engine/bratseth/unbalanced-with-few-docs

Bratseth/unbalanced with few docs

5 files changed, 88 insertions, 60 deletions

diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
index 943390cb10c..be761acf2c2 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
@@ -157,7 +157,6 @@ public class DispatcherTest {
         @Override
         public Optional<SearchInvoker> createSearchInvoker(VespaBackEndSearcher searcher,
                                                            Query query,
-                                                           OptionalInt groupId,
                                                            List<Node> nodes,
                                                            boolean acceptIncompleteCoverage,
                                                            int maxHitsPerNode) {
@@ -167,7 +166,7 @@ public class DispatcherTest {
             boolean nonEmpty = events[step].returnInvoker(nodes, acceptIncompleteCoverage);
             step++;
             if (nonEmpty) {
-                return Optional.of(new MockInvoker(nodes.get(0).key(), groupId));
+                return Optional.of(new MockInvoker(nodes.get(0).key()));
             } else {
                 return Optional.empty();
             }
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java
index 730aa0800e7..21a15165ab3 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/InterleavedSearchInvokerTest.java
@@ -7,6 +7,8 @@ import com.yahoo.prelude.fastsearch.FastHit;
 import com.yahoo.prelude.fastsearch.GroupingListHit;
 import com.yahoo.search.Query;
 import com.yahoo.search.Result;
+import com.yahoo.search.dispatch.searchcluster.Group;
+import com.yahoo.search.dispatch.searchcluster.Node;
 import com.yahoo.search.dispatch.searchcluster.SearchCluster;
 import com.yahoo.search.result.Coverage;
 import com.yahoo.search.result.DefaultErrorHit;
@@ -53,7 +55,7 @@ public class InterleavedSearchInvokerTest {
     @Test
     public void requireThatAdaptiveTimeoutsAreNotUsedWithFullCoverageRequirement() throws IOException {
         SearchCluster cluster = new MockSearchCluster("!", createDispatchConfig(100.0), 1, 3);
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 3);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 3);
 
         expectedEvents.add(new Event(5000, 100, 0));
         expectedEvents.add(new Event(4900, 100, 1));
@@ -67,7 +69,7 @@ public class InterleavedSearchInvokerTest {
     @Test
     public void requireThatTimeoutsAreNotMarkedAsAdaptive() throws IOException {
         SearchCluster cluster = new MockSearchCluster("!", createDispatchConfig(100.0), 1, 3);
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 3);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 3);
 
         expectedEvents.add(new Event(5000, 300, 0));
         expectedEvents.add(new Event(4700, 300, 1));
@@ -85,7 +87,7 @@ public class InterleavedSearchInvokerTest {
     @Test
     public void requireThatAdaptiveTimeoutDecreasesTimeoutWhenCoverageIsReached() throws IOException {
         SearchCluster cluster = new MockSearchCluster("!", createDispatchConfig(50.0), 1, 4);
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 4);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 4);
 
         expectedEvents.add(new Event(5000, 100, 0));
         expectedEvents.add(new Event(4900, 100, 1));
@@ -106,7 +108,7 @@ public class InterleavedSearchInvokerTest {
         SearchCluster cluster = new MockSearchCluster("!", 1, 2);
         invokers.add(new MockInvoker(0, createCoverage(50155, 50155, 50155, 1, 1, 0)));
         invokers.add(new MockInvoker(1, createCoverage(49845, 49845, 49845, 1, 1, 0)));
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 0);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0);
 
         expectedEvents.add(new Event(null, 100, 0));
         expectedEvents.add(new Event(null, 200, 1));
@@ -127,7 +129,7 @@ public class InterleavedSearchInvokerTest {
         SearchCluster cluster = new MockSearchCluster("!", 1, 2);
         invokers.add(new MockInvoker(0, createCoverage(10101, 50155, 50155, 1, 1, DEGRADED_BY_MATCH_PHASE)));
         invokers.add(new MockInvoker(1, createCoverage(13319, 49845, 49845, 1, 1, DEGRADED_BY_MATCH_PHASE)));
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 0);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0);
 
         expectedEvents.add(new Event(null, 100, 0));
         expectedEvents.add(new Event(null, 200, 1));
@@ -149,7 +151,7 @@ public class InterleavedSearchInvokerTest {
         SearchCluster cluster = new MockSearchCluster("!", 1, 2);
         invokers.add(new MockInvoker(0, createCoverage(5000, 50155, 50155, 1, 1, DEGRADED_BY_TIMEOUT)));
         invokers.add(new MockInvoker(1, createCoverage(4900, 49845, 49845, 1, 1, DEGRADED_BY_TIMEOUT)));
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 0);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()),0);
 
         expectedEvents.add(new Event(null, 100, 0));
         expectedEvents.add(new Event(null, 200, 1));
@@ -171,7 +173,7 @@ public class InterleavedSearchInvokerTest {
         SearchCluster cluster = new MockSearchCluster("!", 1, 2);
         invokers.add(new MockInvoker(0, createCoverage(50155, 50155, 50155, 1, 1, 0)));
         invokers.add(new MockInvoker(1, createCoverage(49845, 49845, 49845, 1, 1, 0)));
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 0);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0);
 
         expectedEvents.add(new Event(null, 100, 0));
         expectedEvents.add(null);
@@ -205,8 +207,8 @@ public class InterleavedSearchInvokerTest {
     private static final List<Double> A5Aux = Arrays.asList(-1.0,11.0,8.5,7.5,-7.0,3.0,2.0);
     private static final List<Double> B5Aux = Arrays.asList(9.0,8.0,-3.0,7.0,6.0,1.0, -1.0);
 
-    private void validateThatTopKProbabilityOverrideTakesEffect(Double topKProbability, int expectedK, boolean isContentWellBalanced) throws IOException {
-        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, isContentWellBalanced);
+    private void validateThatTopKProbabilityOverrideTakesEffect(Double topKProbability, int expectedK, Group group) throws IOException {
+        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, group);
         query.setHits(8);
         query.properties().set(Dispatcher.topKProbability, topKProbability);
         SearchInvoker [] invokers = invoker.invokers().toArray(new SearchInvoker[0]);
@@ -228,17 +230,37 @@ public class InterleavedSearchInvokerTest {
 
     @Test
     public void requireThatTopKProbabilityOverrideTakesEffect() throws IOException {
-        validateThatTopKProbabilityOverrideTakesEffect(null, 8, true);
-        validateThatTopKProbabilityOverrideTakesEffect(0.8, 7, true);
+        validateThatTopKProbabilityOverrideTakesEffect(null, 8, new Group(0, List.of()));
+        validateThatTopKProbabilityOverrideTakesEffect(0.8, 7, new Group(0, List.of()));
     }
+
     @Test
     public void requireThatTopKProbabilityOverrideIsDisabledOnContentSkew() throws IOException {
-        validateThatTopKProbabilityOverrideTakesEffect(0.8, 8, false);
+        Node node0 = new Node(0, "host0", 0);
+        Node node1 = new Node(1, "host1", 0);
+        Group group = new Group(0, List.of(node0, node1));
+
+        node0.setActiveDocuments(1000000);
+        node1.setActiveDocuments(1100000);
+        group.aggregateNodeValues();
+        validateThatTopKProbabilityOverrideTakesEffect(0.8, 8, group);
+    }
+
+    @Test
+    public void requireThatTopKProbabilityOverrideIsDisabledOnLittleContent() throws IOException {
+        Node node0 = new Node(0, "host0", 0);
+        Node node1 = new Node(1, "host1", 0);
+        Group group = new Group(0, List.of(node0, node1));
+
+        node0.setActiveDocuments(10);
+        node1.setActiveDocuments(10);
+        group.aggregateNodeValues();
+        validateThatTopKProbabilityOverrideTakesEffect(0.8, 8, group);
     }
 
     @Test
     public void requireThatMergeOfConcreteHitsObeySorting() throws IOException {
-        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, true);
+        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, new Group(0, List.of()));
         query.setHits(12);
         Result result = invoker.search(query, null);
         assertEquals(10, result.hits().size());
@@ -247,7 +269,7 @@ public class InterleavedSearchInvokerTest {
         assertEquals(0, result.getQuery().getOffset());
         assertEquals(12, result.getQuery().getHits());
 
-        invoker = createInterLeavedTestInvoker(B5, A5, true);
+        invoker = createInterLeavedTestInvoker(B5, A5, new Group(0, List.of()));
         result = invoker.search(query, null);
         assertEquals(10, result.hits().size());
         assertEquals(11.0, result.hits().get(0).getRelevance().getScore(), DELTA);
@@ -258,7 +280,7 @@ public class InterleavedSearchInvokerTest {
 
     @Test
     public void requireThatMergeOfConcreteHitsObeyOffset() throws IOException {
-        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, true);
+        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5, B5, new Group(0, List.of()));
         query.setHits(3);
         query.setOffset(5);
         Result result = invoker.search(query, null);
@@ -268,7 +290,7 @@ public class InterleavedSearchInvokerTest {
         assertEquals(0, result.getQuery().getOffset());
         assertEquals(3, result.getQuery().getHits());
 
-        invoker = createInterLeavedTestInvoker(B5, A5, true);
+        invoker = createInterLeavedTestInvoker(B5, A5, new Group(0, List.of()));
         query.setOffset(5);
         result = invoker.search(query, null);
         assertEquals(3, result.hits().size());
@@ -280,7 +302,7 @@ public class InterleavedSearchInvokerTest {
 
     @Test
     public void requireThatMergeOfConcreteHitsObeyOffsetWithAuxilliaryStuff() throws IOException {
-        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5Aux, B5Aux, true);
+        InterleavedSearchInvoker invoker = createInterLeavedTestInvoker(A5Aux, B5Aux, new Group(0, List.of()));
         query.setHits(3);
         query.setOffset(5);
         Result result = invoker.search(query, null);
@@ -291,7 +313,7 @@ public class InterleavedSearchInvokerTest {
         assertEquals(0, result.getQuery().getOffset());
         assertEquals(3, result.getQuery().getHits());
 
-        invoker = createInterLeavedTestInvoker(B5Aux, A5Aux, true);
+        invoker = createInterLeavedTestInvoker(B5Aux, A5Aux, new Group(0, List.of()));
         query.setOffset(5);
         result = invoker.search(query, null);
         assertEquals(7, result.hits().size());
@@ -302,13 +324,12 @@ public class InterleavedSearchInvokerTest {
         assertEquals(3, result.getQuery().getHits());
     }
 
-    private static InterleavedSearchInvoker createInterLeavedTestInvoker(List<Double> a, List<Double> b,
-                                                                         boolean isContentWellBalanced) {
+    private static InterleavedSearchInvoker createInterLeavedTestInvoker(List<Double> a, List<Double> b, Group group) {
         SearchCluster cluster = new MockSearchCluster("!", 1, 2);
         List<SearchInvoker> invokers = new ArrayList<>();
         invokers.add(createInvoker(a, 0));
         invokers.add(createInvoker(b, 1));
-        InterleavedSearchInvoker invoker = new InterleavedSearchInvoker(invokers, isContentWellBalanced, cluster, Collections.emptySet());
+        InterleavedSearchInvoker invoker = new InterleavedSearchInvoker(invokers, cluster, group, Collections.emptySet());
         invoker.responseAvailable(invokers.get(0));
         invoker.responseAvailable(invokers.get(1));
         return invoker;
@@ -336,7 +357,7 @@ public class InterleavedSearchInvokerTest {
         Coverage errorCoverage = new Coverage(0, 0, 0);
         errorCoverage.setNodesTried(1);
         invokers.add(new SearchErrorInvoker(ErrorMessage.createBackendCommunicationError("node is down"), errorCoverage));
-        SearchInvoker invoker = createInterleavedInvoker(cluster, 0);
+        SearchInvoker invoker = createInterleavedInvoker(cluster, new Group(0, List.of()), 0);
 
         expectedEvents.add(new Event(null,   1, 1));
         expectedEvents.add(new Event(null, 100, 0));
@@ -354,12 +375,13 @@ public class InterleavedSearchInvokerTest {
         assertThat(cov.isDegradedByTimeout(), is(true));
     }
 
-    private InterleavedSearchInvoker createInterleavedInvoker(SearchCluster searchCluster, int numInvokers) {
+    private InterleavedSearchInvoker createInterleavedInvoker(SearchCluster searchCluster, Group group, int numInvokers) {
         for (int i = 0; i < numInvokers; i++) {
             invokers.add(new MockInvoker(i));
         }
 
-        return new InterleavedSearchInvoker(invokers, false, searchCluster, null) {
+        return new InterleavedSearchInvoker(invokers, searchCluster, group,null) {
+
             @Override
             protected long currentTime() {
                 return clock.millis();
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java b/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java
index 53d1a2457d0..d86fcdfc25d 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java
@@ -4,6 +4,7 @@ package com.yahoo.search.dispatch;
 import com.yahoo.prelude.fastsearch.FastHit;
 import com.yahoo.search.Query;
 import com.yahoo.search.Result;
+import com.yahoo.search.dispatch.searchcluster.Group;
 import com.yahoo.search.dispatch.searchcluster.Node;
 import com.yahoo.search.result.Coverage;
 import com.yahoo.search.result.Hit;
@@ -17,27 +18,17 @@ import java.util.OptionalInt;
 class MockInvoker extends SearchInvoker {
 
     private final Coverage coverage;
-    private final OptionalInt groupId;
     private Query query;
     private List<Hit> hits;
     int hitsRequested;
 
-    protected MockInvoker(int key, Coverage coverage, OptionalInt groupId) {
+    protected MockInvoker(int key, Coverage coverage) {
         super(Optional.of(new Node(key, "?", 0)));
         this.coverage = coverage;
-        this.groupId = groupId;
-    }
-
-    protected MockInvoker(int key, OptionalInt groupId) {
-        this(key, null, groupId);
-    }
-
-    protected MockInvoker(int key, Coverage coverage) {
-        this(key, coverage, OptionalInt.empty());
     }
 
     protected MockInvoker(int key) {
-        this(key, null, OptionalInt.empty());
+        this(key, null);
     }
 
     MockInvoker setHits(List<Hit> hits) {
@@ -45,18 +36,15 @@ class MockInvoker extends SearchInvoker {
         return this;
     }
 
-    /** Returns the group to be invoked, if known */
-    public OptionalInt groupId() { return groupId; }
-
     @Override
-    protected Object sendSearchRequest(Query query, Object context) throws IOException {
+    protected Object sendSearchRequest(Query query, Object context) {
         this.query = query;
         hitsRequested = query.getHits();
         return context;
     }
 
     @Override
-    protected InvokerResult getSearchResult(Execution execution) throws IOException {
+    protected InvokerResult getSearchResult(Execution execution) {
         InvokerResult ret = new InvokerResult(query, 10);
         if (coverage != null) {
             ret.getResult().setCoverage(coverage);
@@ -80,8 +68,7 @@ class MockInvoker extends SearchInvoker {
 
     @Override
     public String toString() {
-        return "invoker with key " + distributionKey() +
-               (groupId().isPresent() ? " of group " + groupId().getAsInt() : "");
+        return "invoker with key " + distributionKey();
     }
 
 }
\ No newline at end of file
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 c9f7469acbb..8101aee74fd 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
@@ -101,7 +101,7 @@ public class SearchClusterCoverageTest {
     }
 
     @Test
-    public void one_group_few_docs_has_well_balanced_content() {
+    public void one_group_few_docs_unbalanced() {
         var tester = new SearchClusterTester(1, 2);
 
         Node node0 = tester.group(0).nodes().get(0);
@@ -115,7 +115,27 @@ public class SearchClusterCoverageTest {
         node1.setActiveDocuments(0);
 
         tester.pingIterationCompleted();
-        assertTrue(tester.group(0).isContentWellBalanced());
+        assertFalse(tester.group(0).isBalanced());
+        assertTrue(tester.group(0).isSparse());
+    }
+
+    @Test
+    public void one_group_many_docs_unbalanced() {
+        var tester = new SearchClusterTester(1, 2);
+
+        Node node0 = tester.group(0).nodes().get(0);
+        Node node1 = tester.group(0).nodes().get(1);
+
+        // 1 document
+        node0.setWorking(true);
+        node1.setWorking(true);
+
+        node0.setActiveDocuments(1000000);
+        node1.setActiveDocuments(100000);
+
+        tester.pingIterationCompleted();
+        assertFalse(tester.group(0).isBalanced());
+        assertFalse(tester.group(0).isSparse());
     }
 
 }
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java
index 48134094faf..f46717ce180 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java
@@ -337,45 +337,45 @@ public class SearchClusterTest {
     @Test
     public void requireThatEmptyGroupIsInBalance() {
         Group group = new Group(0, new ArrayList<>());
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
         group.aggregateNodeValues();
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
     }
 
     @Test
     public void requireThatSingleNodeGroupIsInBalance() {
         Group group = new Group(0, Arrays.asList(new Node(1, "n", 1)));
         group.nodes().forEach(node -> node.setWorking(true));
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
         group.aggregateNodeValues();
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
         group.nodes().get(0).setActiveDocuments(1000);
         group.aggregateNodeValues();
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
     }
 
     @Test
     public void requireThatMultiNodeGroupDetectsBalance() {
         Group group = new Group(0, Arrays.asList(new Node(1, "n1", 1), new Node(2, "n2", 1)));
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
         group.nodes().forEach(node -> node.setWorking(true));
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
         group.aggregateNodeValues();
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
         group.nodes().get(0).setActiveDocuments(1000);
         group.aggregateNodeValues();
-        assertFalse(group.isContentWellBalanced());
+        assertFalse(group.isBalanced());
         group.nodes().get(1).setActiveDocuments(100);
         group.aggregateNodeValues();
-        assertFalse(group.isContentWellBalanced());
+        assertFalse(group.isBalanced());
         group.nodes().get(1).setActiveDocuments(800);
         group.aggregateNodeValues();
-        assertFalse(group.isContentWellBalanced());
+        assertFalse(group.isBalanced());
         group.nodes().get(1).setActiveDocuments(818);
         group.aggregateNodeValues();
-        assertFalse(group.isContentWellBalanced());
+        assertFalse(group.isBalanced());
         group.nodes().get(1).setActiveDocuments(819);
         group.aggregateNodeValues();
-        assertTrue(group.isContentWellBalanced());
+        assertTrue(group.isBalanced());
     }
 }