Remove leftovers from dispatching through fdispatch

5 files changed, 61 insertions, 98 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java
index 1a373b6ea71..c04553ae2f5 100644
--- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java
@@ -147,7 +147,7 @@ public class FastSearcher extends VespaBackEndSearcher {
      * on the same host.
      */
     private SearchInvoker getSearchInvoker(Query query) {
-        return dispatcher.getSearchInvoker(query, this).get();
+        return dispatcher.getSearchInvoker(query, this);
     }
 
     /**
@@ -156,11 +156,9 @@ public class FastSearcher extends VespaBackEndSearcher {
      * content nodes.
      */
     private FillInvoker getFillInvoker(Result result) {
-        return dispatcher.getFillInvoker(result, this).get();
+        return dispatcher.getFillInvoker(result, this);
     }
 
-
-
     private static Optional<String> quotedSummaryClass(String summaryClass) {
         return Optional.of(summaryClass == null ? "[null]" : quote(summaryClass));
     }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
index 1f58a2df5c2..ab010001d90 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
@@ -49,7 +49,6 @@ public class Dispatcher extends AbstractComponent {
     private static final String INTERNAL = "internal";
     private static final String PROTOBUF = "protobuf";
 
-    private static final String FDISPATCH_METRIC = "dispatch_fdispatch";
     private static final String INTERNAL_METRIC = "dispatch_internal";
 
     private static final int MAX_GROUP_SELECTION_ATTEMPTS = 3;
@@ -61,7 +60,6 @@ public class Dispatcher extends AbstractComponent {
     private final SearchCluster searchCluster;
 
     private final LoadBalancer loadBalancer;
-    private final boolean multilevelDispatch;
 
     private final InvokerFactory invokerFactory;
 
@@ -115,11 +113,13 @@ public class Dispatcher extends AbstractComponent {
                          InvokerFactory invokerFactory,
                          PingFactory pingFactory,
                          Metric metric) {
+        if (dispatchConfig.useMultilevelDispatch())
+            throw new IllegalArgumentException(searchCluster + " is configured with multilevel dispatch, but this is not supported");
+
         this.searchCluster = searchCluster;
         this.loadBalancer = new LoadBalancer(searchCluster,
                                   dispatchConfig.distributionPolicy() == DispatchConfig.DistributionPolicy.ROUNDROBIN);
         this.invokerFactory = invokerFactory;
-        this.multilevelDispatch = dispatchConfig.useMultilevelDispatch();
         this.metric = metric;
         this.metricContext = metric.createContext(null);
 
@@ -137,27 +137,18 @@ public class Dispatcher extends AbstractComponent {
         searchCluster.shutDown();
     }
 
-    public Optional<FillInvoker> getFillInvoker(Result result, VespaBackEndSearcher searcher) {
+    public FillInvoker getFillInvoker(Result result, VespaBackEndSearcher searcher) {
         return invokerFactory.createFillInvoker(searcher, result);
     }
 
-    public Optional<SearchInvoker> getSearchInvoker(Query query, VespaBackEndSearcher searcher) {
-        if (multilevelDispatch) {
-            emitDispatchMetric(Optional.empty());
-            return Optional.empty();
-        }
-
-        Optional<SearchInvoker> invoker = getSearchPathInvoker(query, searcher);
+    public SearchInvoker getSearchInvoker(Query query, VespaBackEndSearcher searcher) {
+        SearchInvoker invoker = getSearchPathInvoker(query, searcher).orElseGet(() -> getInternalInvoker(query, searcher));
 
-        if (invoker.isEmpty()) {
-            invoker = getInternalInvoker(query, searcher);
-        }
-        if (invoker.isPresent() && query.properties().getBoolean(com.yahoo.search.query.Model.ESTIMATE)) {
+        if (query.properties().getBoolean(com.yahoo.search.query.Model.ESTIMATE)) {
             query.setHits(0);
             query.setOffset(0);
         }
-        emitDispatchMetric(invoker);
-
+        metric.add(INTERNAL_METRIC, 1, metricContext);
         return invoker;
     }
 
@@ -177,12 +168,13 @@ public class Dispatcher extends AbstractComponent {
         }
     }
 
-    private Optional<SearchInvoker> getInternalInvoker(Query query, VespaBackEndSearcher searcher) {
+    private SearchInvoker getInternalInvoker(Query query, VespaBackEndSearcher searcher) {
         Optional<Node> directNode = searchCluster.localCorpusDispatchTarget();
         if (directNode.isPresent()) {
             Node node = directNode.get();
-            query.trace(false, 2, "Dispatching directly to ", node);
-            return invokerFactory.createSearchInvoker(searcher, query, OptionalInt.empty(), Arrays.asList(node), true);
+            query.trace(false, 2, "Dispatching to ", node);
+            return invokerFactory.createSearchInvoker(searcher, query, OptionalInt.empty(), Arrays.asList(node), true)
+                                 .orElseThrow(() -> new IllegalStateException("Could not dispatch directly to " + node));
         }
 
         int covered = searchCluster.groupsWithSufficientCoverage();
@@ -201,10 +193,10 @@ public class Dispatcher extends AbstractComponent {
                                                                                  group.nodes(),
                                                                                  acceptIncompleteCoverage);
             if (invoker.isPresent()) {
-                query.trace(false, 2, "Dispatching internally to search group ", group.id());
+                query.trace(false, 2, "Dispatching to group ", group.id());
                 query.getModel().setSearchPath("/" + group.id());
                 invoker.get().teardown((success, time) -> loadBalancer.releaseGroup(group, success, time));
-                return invoker;
+                return invoker.get();
             } else {
                 loadBalancer.releaseGroup(group, false, 0);
                 if (rejected == null) {
@@ -213,16 +205,7 @@ public class Dispatcher extends AbstractComponent {
                 rejected.add(group.id());
             }
         }
-
-        return Optional.empty();
-    }
-
-    private void emitDispatchMetric(Optional<SearchInvoker> invoker) {
-        if (invoker.isEmpty()) {
-            metric.add(FDISPATCH_METRIC, 1, metricContext);
-        } else {
-            metric.add(INTERNAL_METRIC, 1, metricContext);
-        }
+        throw new IllegalStateException("No suitable groups to dispatch query. Rejected: " + rejected);
     }
 
 }
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 78c50254a84..6030e989595 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
@@ -20,6 +20,7 @@ import java.util.Set;
  * @author ollivir
  */
 public abstract class InvokerFactory {
+
     protected final SearchCluster searchCluster;
 
     public InvokerFactory(SearchCluster searchCluster) {
@@ -28,24 +29,18 @@ public abstract class InvokerFactory {
 
     protected abstract Optional<SearchInvoker> createNodeSearchInvoker(VespaBackEndSearcher searcher, Query query, Node node);
 
-    public abstract Optional<FillInvoker> createFillInvoker(VespaBackEndSearcher searcher, Result result);
+    public abstract FillInvoker createFillInvoker(VespaBackEndSearcher searcher, Result result);
 
     /**
      * Create a {@link SearchInvoker} for a list of content nodes.
      *
-     * @param searcher
-     *            the searcher processing the query
-     * @param query
-     *            the search query being processed
-     * @param groupId
-     *            the id of the node group to which the nodes belong
-     * @param nodes
-     *            pre-selected list of content nodes
-     * @param acceptIncompleteCoverage
-     *            if some of the nodes are unavailable and this parameter is
-     *            <b>false</b>, verify that the remaining set of nodes has enough
-     *            coverage
-     * @return Optional containing the SearchInvoker or <i>empty</i> if some node in the
+     * @param searcher the searcher processing the query
+     * @param query the search query being processed
+     * @param groupId the id of the node group to which the nodes belong
+     * @param nodes pre-selected list of content nodes
+     * @param acceptIncompleteCoverage if some of the nodes are unavailable and this parameter is
+     *                                 false, verify that the remaining set of nodes has sufficient coverage
+     * @return Optional containing the SearchInvoker or empty if some node in the
      *         list is invalid and the remaining coverage is not sufficient
      */
     public Optional<SearchInvoker> createSearchInvoker(VespaBackEndSearcher searcher,
@@ -76,11 +71,11 @@ public abstract class InvokerFactory {
         if (failed != null) {
             List<Node> success = new ArrayList<>(nodes.size() - failed.size());
             for (Node node : nodes) {
-                if (!failed.contains(node.key())) {
+                if ( ! failed.contains(node.key())) {
                     success.add(node);
                 }
             }
-            if (!searchCluster.isPartialGroupCoverageSufficient(groupId, success) && !acceptIncompleteCoverage) {
+            if ( ! searchCluster.isPartialGroupCoverageSufficient(groupId, success) && !acceptIncompleteCoverage) {
                 return Optional.empty();
             }
             if(invokers.size() == 0) {
@@ -113,4 +108,5 @@ public abstract class InvokerFactory {
     }
 
     public void release() {}
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcInvokerFactory.java b/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcInvokerFactory.java
index 6160d3dfa08..870f7aef9c5 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcInvokerFactory.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcInvokerFactory.java
@@ -40,7 +40,7 @@ public class RpcInvokerFactory extends InvokerFactory implements PingFactory {
     }
 
     @Override
-    public Optional<FillInvoker> createFillInvoker(VespaBackEndSearcher searcher, Result result) {
+    public FillInvoker createFillInvoker(VespaBackEndSearcher searcher, Result result) {
         Query query = result.getQuery();
 
         boolean summaryNeedsQuery = searcher.summaryNeedsQuery(query);
@@ -48,8 +48,8 @@ public class RpcInvokerFactory extends InvokerFactory implements PingFactory {
         boolean useDispatchDotSummaries = query.properties().getBoolean(dispatchSummaries, false);
 
         return  ((useDispatchDotSummaries || !useProtoBuf) && ! summaryNeedsQuery)
-                ? Optional.of(new RpcFillInvoker(rpcResourcePool, searcher.getDocumentDatabase(query)))
-                : Optional.of(new RpcProtobufFillInvoker(rpcResourcePool, searcher.getDocumentDatabase(query), searcher.getServerId(), summaryNeedsQuery));
+                ? new RpcFillInvoker(rpcResourcePool, searcher.getDocumentDatabase(query))
+                : new RpcProtobufFillInvoker(rpcResourcePool, searcher.getDocumentDatabase(query), searcher.getServerId(), summaryNeedsQuery);
     }
 
     // for testing
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 30f6c5a495d..291b0f4890a 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
@@ -11,7 +11,6 @@ import com.yahoo.search.cluster.ClusterMonitor;
 import com.yahoo.search.dispatch.searchcluster.Node;
 import com.yahoo.search.dispatch.searchcluster.PingFactory;
 import com.yahoo.search.dispatch.searchcluster.SearchCluster;
-import com.yahoo.vespa.config.search.DispatchConfig;
 import org.junit.Test;
 
 import java.util.List;
@@ -20,46 +19,28 @@ import java.util.OptionalInt;
 import java.util.concurrent.Callable;
 
 import static com.yahoo.search.dispatch.MockSearchCluster.createDispatchConfig;
-import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 /**
  * @author ollivir
  */
 public class DispatcherTest {
-    private static final CompoundName internalDispatch = new CompoundName("dispatch.internal");
-
-    private static Query query() {
-        Query q = new Query();
-        q.properties().set(internalDispatch, "true");
-        return q;
-    }
-
-    @Test
-    public void requireDispatcherToIgnoreMultilevelConfigurations() {
-        SearchCluster searchCluster = new MockSearchCluster("1", 2, 2);
-        DispatchConfig dispatchConfig = new DispatchConfig.Builder().useMultilevelDispatch(true).build();
-
-        var invokerFactory = new MockInvokerFactory(searchCluster);
-
-        Dispatcher disp = new Dispatcher(searchCluster, dispatchConfig, invokerFactory, invokerFactory, new MockMetric());
-        assertThat(disp.getSearchInvoker(query(), null).isPresent(), is(false));
-    }
 
     @Test
     public void requireThatDispatcherSupportsSearchPath() {
         SearchCluster cl = new MockSearchCluster("1", 2, 2);
-        Query q = query();
+        Query q = new Query();
         q.getModel().setSearchPath("1/0"); // second node in first group
         MockInvokerFactory invokerFactory = new MockInvokerFactory(cl, (nodes, a) -> {
-            assertThat(nodes.size(), is(1));
-            assertThat(nodes.get(0).key(), is(2));
+            assertEquals(1, nodes.size());
+            assertEquals(2, nodes.get(0).key());
             return true;
         });
         Dispatcher disp = new Dispatcher(cl, createDispatchConfig(), invokerFactory, invokerFactory, new MockMetric());
-        Optional<SearchInvoker> invoker = disp.getSearchInvoker(q, null);
-        assertThat(invoker.isPresent(), is(true));
+        SearchInvoker invoker = disp.getSearchInvoker(q, null);
         invokerFactory.verifyAllEventsProcessed();
     }
 
@@ -73,8 +54,7 @@ public class DispatcherTest {
         };
         MockInvokerFactory invokerFactory = new MockInvokerFactory(cl, (n, a) -> true);
         Dispatcher disp = new Dispatcher(cl, createDispatchConfig(), invokerFactory, invokerFactory, new MockMetric());
-        Optional<SearchInvoker> invoker = disp.getSearchInvoker(query(), null);
-        assertThat(invoker.isPresent(), is(true));
+        SearchInvoker invoker = disp.getSearchInvoker(new Query(), null);
         invokerFactory.verifyAllEventsProcessed();
     }
 
@@ -83,31 +63,34 @@ public class DispatcherTest {
         SearchCluster cl = new MockSearchCluster("1", 2, 1);
 
         MockInvokerFactory invokerFactory = new MockInvokerFactory(cl, (n, acceptIncompleteCoverage) -> {
-            assertThat(acceptIncompleteCoverage, is(false));
+            assertFalse(acceptIncompleteCoverage);
             return false;
         }, (n, acceptIncompleteCoverage) -> {
-            assertThat(acceptIncompleteCoverage, is(true));
+            assertTrue(acceptIncompleteCoverage);
             return true;
         });
         Dispatcher disp = new Dispatcher(cl, createDispatchConfig(), invokerFactory, invokerFactory, new MockMetric());
-        Optional<SearchInvoker> invoker = disp.getSearchInvoker(query(), null);
-        assertThat(invoker.isPresent(), is(true));
+        SearchInvoker invoker = disp.getSearchInvoker(new Query(), null);
         invokerFactory.verifyAllEventsProcessed();
     }
 
     @Test
     public void requireThatInvokerConstructionDoesNotRepeatGroups() {
-        SearchCluster cl = new MockSearchCluster("1", 2, 1);
+        try {
+            SearchCluster cl = new MockSearchCluster("1", 2, 1);
 
-        MockInvokerFactory invokerFactory = new MockInvokerFactory(cl, (n, a) -> false, (n, a) -> false);
-        Dispatcher disp = new Dispatcher(cl, createDispatchConfig(), invokerFactory, invokerFactory, new MockMetric());
-        Optional<SearchInvoker> invoker = disp.getSearchInvoker(query(), null);
-        assertThat(invoker.isPresent(), is(false));
-        invokerFactory.verifyAllEventsProcessed();
+            MockInvokerFactory invokerFactory = new MockInvokerFactory(cl, (n, a) -> false, (n, a) -> false);
+            Dispatcher disp = new Dispatcher(cl, createDispatchConfig(), invokerFactory, invokerFactory, new MockMetric());
+            disp.getSearchInvoker(new Query(), null);
+            fail("Expected exception");
+        }
+        catch (IllegalStateException e) {
+            assertEquals("No suitable groups to dispatch query. Rejected: [0, 1]", e.getMessage());
+        }
     }
 
     interface FactoryStep {
-        public boolean returnInvoker(List<Node> nodes, boolean acceptIncompleteCoverage);
+        boolean returnInvoker(List<Node> nodes, boolean acceptIncompleteCoverage);
     }
 
     private static class MockInvokerFactory extends InvokerFactory implements PingFactory {
@@ -121,8 +104,11 @@ public class DispatcherTest {
         }
 
         @Override
-        public Optional<SearchInvoker> createSearchInvoker(VespaBackEndSearcher searcher, Query query, OptionalInt groupId,
-                List<Node> nodes, boolean acceptIncompleteCoverage) {
+        public Optional<SearchInvoker> createSearchInvoker(VespaBackEndSearcher searcher,
+                                                           Query query,
+                                                           OptionalInt groupId,
+                                                           List<Node> nodes,
+                                                           boolean acceptIncompleteCoverage) {
             if (step >= events.length) {
                 throw new RuntimeException("Was not expecting more calls to getSearchInvoker");
             }
@@ -136,7 +122,7 @@ public class DispatcherTest {
         }
 
         void verifyAllEventsProcessed() {
-            assertThat(step, is(events.length));
+            assertEquals(events.length, step);
         }
 
         @Override
@@ -146,7 +132,7 @@ public class DispatcherTest {
         }
 
         @Override
-        public Optional<FillInvoker> createFillInvoker(VespaBackEndSearcher searcher, Result result) {
+        public FillInvoker createFillInvoker(VespaBackEndSearcher searcher, Result result) {
             fail("Unexpected call to createFillInvoker");
             return null;
         }