Revert "dispatch.summaries by default when possible"

5 files changed, 34 insertions, 45 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 df0ed02b70c..7b5ded6a9d6 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
@@ -56,9 +56,6 @@ public class FastSearcher extends VespaBackEndSearcher {
     /** If this is turned on this will make search queries directly to the local search node when possible */
     private final static CompoundName dispatchDirect = new CompoundName("dispatch.direct");
 
-    /** Unless turned off this will fill summaries by dispatching directly to search nodes over RPC when possible */
-    private final static CompoundName dispatchSummaries = new CompoundName("dispatch.summaries");
-
     /** The compression method which will be used with rpc dispatch. "lz4" (default) and "none" is supported. */
     private final static CompoundName dispatchCompression = new CompoundName("dispatch.compression");
 
@@ -265,7 +262,7 @@ public class FastSearcher extends VespaBackEndSearcher {
         Query query = result.getQuery();
         traceQuery(getName(), "fill", query, query.getOffset(), query.getHits(), 2, quotedSummaryClass(summaryClass));
 
-        if (query.properties().getBoolean(dispatchSummaries, true) && !summaryNeedsQuery(query)) {
+        if (wantsRPCSummaryFill(query)) {
             CompressionType compression =
                 CompressionType.valueOf(query.properties().getString(dispatchCompression, "LZ4").toUpperCase());
             fillSDDocName(result);
diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java
index e5c9f048e53..eac1579a821 100644
--- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java
@@ -55,6 +55,8 @@ public abstract class VespaBackEndSearcher extends PingableSearcher {
 
     private static final CompoundName grouping=new CompoundName("grouping");
     private static final CompoundName combinerows=new CompoundName("combinerows");
+    /** If this is turned on this will fill summaries by dispatching directly to search nodes over RPC */
+    private final static CompoundName dispatchSummaries = new CompoundName("dispatch.summaries");
 
     protected static final CompoundName PACKET_COMPRESSION_LIMIT = new CompoundName("packetcompressionlimit");
     protected static final CompoundName PACKET_COMPRESSION_TYPE = new CompoundName("packetcompressiontype");
@@ -108,6 +110,10 @@ public abstract class VespaBackEndSearcher extends PingableSearcher {
 
     protected abstract void doPartialFill(Result result, String summaryClass);
 
+    protected static boolean wantsRPCSummaryFill(Query query) {
+        return query.properties().getBoolean(dispatchSummaries);
+    }
+
     /**
      * Returns whether we need to send the query when fetching summaries.
      * This is necessary if the query requests summary features or dynamic snippeting
@@ -210,6 +216,12 @@ public abstract class VespaBackEndSearcher extends PingableSearcher {
             return new Result(query, ErrorMessage.createNullQuery(query.getHttpRequest().getUri().toString()));
         }
 
+        if (wantsRPCSummaryFill(query) && summaryNeedsQuery(query)) {
+            return new Result(query, ErrorMessage.createInvalidQueryParameter(
+                    "When using dispatch.summaries and your summary/rankprofile require the query, " +
+                    " you need to enable ranking.queryCache."));
+        }
+
         QueryRewrite.optimizeByRestrict(query);
         QueryRewrite.optimizeAndNot(query);
         QueryRewrite.collapseSingleComposites(query);
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 9f68d327ade..1b775d2c46f 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
@@ -38,11 +38,7 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
- * A dispatcher communicates with search nodes to perform queries and fill hits.
- *
- * This is currently not functionally complete: Queries can only be dispatched to a single node,
- * and summaries can only be requested when they do not need the query.
- *
+ * A dispatcher communicates with search nodes to (in the future) perform queries and (now) fill hits.
  * This class is multithread safe.
  *
  * @author bratseth
diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java
index 01c4a488acd..20150fc2671 100644
--- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java
@@ -4,7 +4,6 @@ package com.yahoo.prelude.fastsearch.test;
 import com.google.common.collect.ImmutableList;
 import com.yahoo.component.chain.Chain;
 import com.yahoo.config.subscription.ConfigGetter;
-import com.yahoo.container.handler.VipStatus;
 import com.yahoo.container.search.Fs4Config;
 import com.yahoo.fs4.mplex.*;
 import com.yahoo.fs4.test.QueryTestCase;
@@ -15,7 +14,6 @@ import com.yahoo.prelude.fastsearch.DocumentdbInfoConfig;
 import com.yahoo.container.protect.Error;
 import com.yahoo.fs4.*;
 import com.yahoo.prelude.fastsearch.test.fs4mock.MockBackend;
-import com.yahoo.prelude.fastsearch.test.fs4mock.MockFS4ResourcePool;
 import com.yahoo.prelude.fastsearch.test.fs4mock.MockFSChannel;
 import com.yahoo.processing.execution.Execution.Trace;
 import com.yahoo.search.Query;
@@ -113,45 +111,30 @@ public class FastSearcherTestCase {
                         .summaryclass(new DocumentdbInfoConfig.Documentdb.Summaryclass.Builder().name("simple").id(7))
                         .rankprofile(new DocumentdbInfoConfig.Documentdb.Rankprofile.Builder()
                                 .name("simpler").hasRankFeatures(false).hasSummaryFeatures(false))));
-
-        List<SearchCluster.Node> nodes = new ArrayList<>();
-        nodes.add(new SearchCluster.Node("host1", 5000, 0));
-        nodes.add(new SearchCluster.Node("host2", 5000, 0));
-
-        MockFS4ResourcePool mockFs4ResourcePool = new MockFS4ResourcePool();
         FastSearcher fastSearcher = new FastSearcher(new MockBackend(),
-                mockFs4ResourcePool,
-                new MockDispatcher(nodes, mockFs4ResourcePool, 1, new VipStatus()),
+                new FS4ResourcePool(1),
+                new MockDispatcher(Collections.emptyList()),
                 new SummaryParameters(null),
                 new ClusterParams("testhittype"),
                 new CacheParams(100, 1e64),
                 documentdbConfigWithOneDb);
 
-        { // No direct.summaries
-            String query = "?query=sddocname:a&summary=simple";
-            Result result = doSearch(fastSearcher, new Query(query), 0, 10);
-            doFill(fastSearcher, result);
-            ErrorMessage error = result.hits().getError();
-            assertNull("Since we don't route to the dispatcher we hit the mock backend, so no error", error);
-        }
+        String query = "?query=sddocname:a&dispatch.summaries";
+        Result result = doSearch(fastSearcher,new Query(query), 0, 10);
+        ErrorMessage message = result.hits().getError();
 
-        { // direct.summaries due to query cache
-            String query = "?query=sddocname:a&ranking.queryCache";
-            Result result = doSearch(fastSearcher, new Query(query), 0, 10);
-            doFill(fastSearcher, result);
-            ErrorMessage error = result.hits().getError();
-            assertEquals("Since we don't actually run summary backends we get this error when the Dispatcher is used",
-                         "Error response from rpc node connection to host1:0: Connection error", error.getDetailedMessage());
-        }
+        assertNotNull("Got error", message);
+        assertEquals("Invalid query parameter", message.getMessage());
+        assertEquals("When using dispatch.summaries and your summary/rankprofile require the query,  you need to enable ranking.queryCache.", message.getDetailedMessage());
+        assertEquals(Error.INVALID_QUERY_PARAMETER.code, message.getCode());
 
-        { // direct.summaries due to no summary features
-            String query = "?query=sddocname:a&dispatch.summaries&summary=simple&ranking=simpler";
-            Result result = doSearch(fastSearcher, new Query(query), 0, 10);
-            doFill(fastSearcher, result);
-            ErrorMessage error = result.hits().getError();
-            assertEquals("Since we don't actually run summary backends we get this error when the Dispatcher is used",
-                         "Error response from rpc node connection to host1:0: Connection error", error.getDetailedMessage());
-        }
+        query = "?query=sddocname:a&dispatch.summaries&ranking.queryCache";
+        result = doSearch(fastSearcher,new Query(query), 0, 10);
+        assertNull(result.hits().getError());
+
+        query = "?query=sddocname:a&dispatch.summaries&summary=simple&ranking=simpler";
+        result = doSearch(fastSearcher,new Query(query), 0, 10);
+        assertNull(result.hits().getError());
     }
 
     @Test
@@ -298,7 +281,7 @@ public class FastSearcherTestCase {
 
         assertEquals(100, fastSearcher.getCacheControl().capacity()); // Default cache =100MB
 
-        Query query = new Query("?query=ignored&dispatch.summaries=false");
+        Query query = new Query("?query=ignored");
         query.getRanking().setQueryCache(true);
         Result result = doSearch(fastSearcher, query, 0, 10);
 
@@ -338,6 +321,7 @@ public class FastSearcherTestCase {
         answer.flip();
         answer.get(expected);
 
+        assertEquals(expected.length, actual.length);
         for (int i = 0; i < expected.length; ++i) {
             if (expected[i] == IGNORE) {
                 actual[i] = IGNORE;
diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/fs4mock/MockFSChannel.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/fs4mock/MockFSChannel.java
index 37a74831e56..8d705406774 100644
--- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/fs4mock/MockFSChannel.java
+++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/fs4mock/MockFSChannel.java
@@ -96,13 +96,13 @@ public class MockFSChannel extends FS4Channel {
                     && lastQueryPacket.getLastOffset() >= 1) {
                 result.addDocument(
                         new DocumentInfo(DocsumDefinitionTestCase.createGlobalId(123),
-                                         2003, 234, 0));
+                                         2003, 234, 1000));
             }
             if (lastQueryPacket.getOffset() <= 1
                     && lastQueryPacket.getLastOffset() >= 2) {
                 result.addDocument(
                         new DocumentInfo(DocsumDefinitionTestCase.createGlobalId(456),
-                                         1855, 234, 1));
+                                1855, 234, 1001));
             }
             packets.add(result);
         }