Preserve federation structure in Result on timeout

9 files changed, 56 insertions, 41 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/Query.java b/container-search/src/main/java/com/yahoo/search/Query.java
index a5007c9cc33..82dac06c424 100644
--- a/container-search/src/main/java/com/yahoo/search/Query.java
+++ b/container-search/src/main/java/com/yahoo/search/Query.java
@@ -544,7 +544,7 @@ public class Query extends com.yahoo.processing.Request implements Cloneable {
 
     /**
      * Sets the number of milliseconds to wait for a response from a search backend
-     * before time out. Default is 500.
+     * before time out. Default is 500 ms.
      */
     public void setTimeout(long timeout) {
         if (timeout > 1000000000 || timeout < 0)
diff --git a/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java b/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java
index b32eec876cc..499cb634295 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java
@@ -206,12 +206,8 @@ public class FederationSearcher extends ForkingSearcher {
     }
 
     private void search(Query query, Execution execution, Target target, Result mergedResults) {
-        Optional<Result> result = search(query, execution, target);
-        if (result.isPresent()) {
-            mergeResult(query, target, mergedResults, result.get());
-        } else {
-            addSearchChainTimedOutError(query, target.getId());
-        }
+        mergeResult(query, target, mergedResults, search(query, execution, target).orElse(createSearchChainTimedOutResult(query, target)));
+
     }
 
     private void search(Query query, Execution execution, Collection<Target> targets, Result mergedResults) {
@@ -220,22 +216,16 @@ public class FederationSearcher extends ForkingSearcher {
 
         HitOrderer s = null;
         for (FederationResult.TargetResult targetResult : results.all()) {
-            if ( ! targetResult.successfullyCompleted()) {
-                addSearchChainTimedOutError(query, targetResult.target.getId());
-            } else {
-                if (s == null) {
-                    s = dirtyCopyIfModifiedOrderer(mergedResults.hits(), targetResult.getOrTimeoutError().hits().getOrderer());
-                }
-                mergeResult(query, targetResult.target, mergedResults, targetResult.getOrTimeoutError());
-            }
+            if (s == null)
+                s = dirtyCopyIfModifiedOrderer(mergedResults.hits(), targetResult.getOrTimeoutError().hits().getOrderer());
+            mergeResult(query, targetResult.target, mergedResults, targetResult.getOrTimeoutError());
         }
     }
 
     private Optional<Result> search(Query query, Execution execution, Target target) {
         long timeout = target.federationOptions().getSearchChainExecutionTimeoutInMilliseconds(query.getTimeLeft());
-        if (timeout <= 0) {
-            return Optional.empty();
-        }
+        if (timeout <= 0) return Optional.empty();
+
         Execution newExecution = new Execution(target.getChain(), execution.context());
         if (strictSearchchain) {
             query.resetTimeout();
@@ -559,10 +549,11 @@ public class FederationSearcher extends ForkingSearcher {
         return target.federationOptions().getRequestTimeoutInMilliseconds() > query.getTimeout();
     }
 
-    private static void addSearchChainTimedOutError(Query query, ComponentId searchChainId) {
-        ErrorMessage timeoutMessage = ErrorMessage.createTimeout("The search chain '" + searchChainId + "' timed out.");
-        timeoutMessage.setSource(searchChainId.stringValue());
-        query.errors().add(timeoutMessage);
+    private static Result createSearchChainTimedOutResult(Query query, Target target) {
+        ErrorMessage timeoutMessage = ErrorMessage.createTimeout("Error in execution of chain '" + target.getId() +
+                                                                 "': " + "Chain timed out.");
+        timeoutMessage.setSource(target.getId().stringValue());
+        return new Result(query, timeoutMessage);
     }
 
     private void mergeResult(Query query, Target target, Result mergedResults, Result result) {
diff --git a/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java b/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java
index 5427da6c06c..e2c79fa5a7e 100644
--- a/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java
+++ b/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java
@@ -407,8 +407,6 @@ public class Execution extends com.yahoo.processing.execution.Execution {
      */
     private final Context[] contextCache;
 
-    private static final Logger log = Logger.getLogger(Execution.class.getName());
-
     /**
      * <p>
      * Creates an execution from another. This execution will start at the
@@ -569,7 +567,7 @@ public class Execution extends com.yahoo.processing.execution.Execution {
 
         try {
             nextProcessor();
-            onInvokingFill(current, result, summaryClass);
+            onInvokingFill(current, summaryClass);
             current.ensureFilled(result, summaryClass, this);
         }
         finally {
@@ -579,7 +577,7 @@ public class Execution extends com.yahoo.processing.execution.Execution {
         }
     }
 
-    private void onInvokingFill(Searcher searcher, Result result, String summaryClass) {
+    private void onInvokingFill(Searcher searcher, String summaryClass) {
         int traceFillAt = 5;
         if (trace().getTraceLevel() < traceFillAt) return;
         trace().trace("Invoke fill(" + summaryClass + ") on " + searcher, traceFillAt);
diff --git a/container-search/src/main/java/com/yahoo/search/searchchain/FutureResult.java b/container-search/src/main/java/com/yahoo/search/searchchain/FutureResult.java
index 440cd935c9f..453b49cfe71 100644
--- a/container-search/src/main/java/com/yahoo/search/searchchain/FutureResult.java
+++ b/container-search/src/main/java/com/yahoo/search/searchchain/FutureResult.java
@@ -91,18 +91,18 @@ public class FutureResult extends FutureTask<Result> {
     }
 
     private ErrorMessage createInterruptedError(Exception e) {
-        return ErrorMessage.createUnspecifiedError("'" + execution + "' was interrupted while executing: " + 
+        return ErrorMessage.createUnspecifiedError(execution + " was interrupted while executing: " +
                                                    Exceptions.toMessageString(e));
     }
     
     private ErrorMessage createExecutionError(Exception e) {
-        log.log(Level.WARNING,"Exception on executing " + execution + " for " + query,e);
+        log.log(Level.WARNING,"Exception in " + execution + " for " + query,e);
         return ErrorMessage.createErrorInPluginSearcher("Error in '" + execution + "': " + Exceptions.toMessageString(e),
                                                         e.getCause());
     }
 
     public ErrorMessage createTimeoutError() {
-        return ErrorMessage.createTimeout("Error executing '" + execution + "': " + " Chain timed out.");
+        return ErrorMessage.createTimeout("Error in " + execution + ": Chain timed out.");
     }
 
 }
diff --git a/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java b/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java
index 0843ec074b6..6eeb425fc9d 100644
--- a/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java
+++ b/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java
@@ -30,7 +30,7 @@ public class FederationOptions implements Cloneable {
      * Creates a fully specified set of options
      *
      * @param optional whether this should be optional
-     * @param timeoutInMilliseconds the max time to wait for a result from this source, or null to not specify a limit
+     * @param timeoutInMilliseconds the max time to wait for a result from this source, or null to use the timeout of the query
      * @param requestTimeoutInMilliseconds the max time to allow this request to live, or null to make this the same as
      *                                     timeoutInMilliseconds. Setting this higher than timeoutInMilliseconds is
      *                                     useful to use queries to populate the cache of slow sources
diff --git a/container-search/src/test/java/com/yahoo/search/federation/test/BlockingSearcher.java b/container-search/src/test/java/com/yahoo/search/federation/test/BlockingSearcher.java
index 7d96638adc9..488b3cc9aac 100644
--- a/container-search/src/test/java/com/yahoo/search/federation/test/BlockingSearcher.java
+++ b/container-search/src/test/java/com/yahoo/search/federation/test/BlockingSearcher.java
@@ -10,6 +10,7 @@ import com.yahoo.search.searchchain.Execution;
  * @author Tony Vaagenes
  */
 public class BlockingSearcher extends Searcher {
+
     @Override
     public synchronized Result search(Query query, Execution execution) {
         try {
@@ -19,4 +20,5 @@ public class BlockingSearcher extends Searcher {
         }
         return execution.search(query);
     }
+
 }
diff --git a/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTest.java b/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTest.java
index c1a2e044381..2d214f9402d 100644
--- a/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTest.java
+++ b/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTest.java
@@ -17,6 +17,7 @@ import com.yahoo.search.federation.selection.FederationTarget;
 import com.yahoo.search.federation.selection.TargetSelector;
 import com.yahoo.search.federation.StrictContractsConfig;
 import com.yahoo.search.result.ErrorHit;
+import com.yahoo.search.result.ErrorMessage;
 import com.yahoo.search.result.Hit;
 import com.yahoo.search.result.HitGroup;
 import com.yahoo.search.searchchain.Execution;
@@ -122,9 +123,26 @@ public class FederationSearcherTest {
         tester.addOptionalSearchChain("chain2", blockingSearcher);
 
         Result result = tester.searchAndFill();
-        assertThat(getNonErrorHits(result).size(), is(1));
-        assertFilled(getFirstHit(getNonErrorHits(result).get(0)));
-        assertNotNull(result.hits().getError());
+        assertEquals(2, result.getHitCount());
+        assertTrue(result.hits().get(0) instanceof HitGroup);
+        assertTrue(result.hits().get(1) instanceof HitGroup);
+        HitGroup chain1Result = (HitGroup)result.hits().get(0);
+        HitGroup chain2Result = (HitGroup)result.hits().get(1);
+
+        // Verify chain1 result: One filled hit
+        assertEquals(1, chain1Result.size());
+        assertFilled(getFirstHit(chain1Result));
+
+        // Verify chain2 result: A timeout error
+        assertEquals(1, chain2Result.size());
+        assertNotNull(chain2Result.getErrorHit());
+        ErrorHit errorHit = chain2Result.getErrorHit();
+        assertEquals(1, errorHit.errors().size());
+        ErrorMessage error = errorHit.errors().iterator().next();
+        assertEquals("chain2", error.getSource());
+        assertEquals(ErrorMessage.timeoutCode, error.getCode());
+        assertEquals("Timed out", error.getMessage());
+        assertEquals("Error in execution of chain 'chain2': Chain timed out.", error.getDetailedMessage());
     }
 
     @Test
@@ -137,8 +155,17 @@ public class FederationSearcherTest {
         Query query = new Query();
         query.setTimeout(50); // make the test run faster
         Result result = tester.search(query);
-        assertThat(getNonErrorHits(result).size(), is(0));
-        assertNotNull(result.hits().getError());
+        assertEquals(1, result.hits().size());
+        assertTrue(result.hits().get(0) instanceof HitGroup);
+        HitGroup chain1Result = (HitGroup)result.hits().get(0);
+        assertEquals(1, chain1Result.size());
+        assertTrue(chain1Result.asList().get(0) instanceof ErrorHit);
+        ErrorHit errorHit = (ErrorHit)chain1Result.get(0);
+        assertEquals(1, errorHit.errors().size());
+        ErrorMessage error = errorHit.errors().iterator().next();
+        assertEquals("chain1", error.getSource());
+        assertEquals(ErrorMessage.timeoutCode, error.getCode());
+        assertEquals("Timed out", error.getMessage());
     }
 
     @Test
diff --git a/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTestCase.java b/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTestCase.java
index da0892f0429..111b7a8eb69 100644
--- a/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/federation/test/FederationSearcherTestCase.java
@@ -71,12 +71,12 @@ public class FederationSearcherTestCase {
     }
 
     @After
-    public void tearDown() throws Exception {
+    public void tearDown() {
         builder = null;
         chainRegistry = null;
     }
 
-    private void addChained(final Searcher searcher, final String sourceName) {
+    private void addChained(Searcher searcher, String sourceName) {
         builder.target(new FederationConfig.Target.Builder().
                 id(sourceName).
                 searchChain(new FederationConfig.Target.SearchChain.Builder().
diff --git a/processing/src/main/java/com/yahoo/processing/execution/Execution.java b/processing/src/main/java/com/yahoo/processing/execution/Execution.java
index 9aff586ef75..ee1e9eb39e4 100644
--- a/processing/src/main/java/com/yahoo/processing/execution/Execution.java
+++ b/processing/src/main/java/com/yahoo/processing/execution/Execution.java
@@ -184,10 +184,7 @@ public class Execution {
     }
 
     public String toString() {
-        StringBuilder s = new StringBuilder("Execution(");
-        s.append(chain.getId());
-        s.append(")#").append(hashCode());
-        return s.toString();
+        return "execution of chain '" + chain.getId() + "'";
     }
 
     public Trace trace() {