diff options
author | Jon Bratseth <bratseth@verizonmedia.com> | 2019-05-07 12:37:41 +0200 |
---|---|---|
committer | Jon Bratseth <bratseth@verizonmedia.com> | 2019-05-07 12:37:41 +0200 |
commit | 35760016e0f038e5696730d227aea8f6ea3e7139 (patch) | |
tree | 2673d3e88468616a8685fef488a0b4ba74aaadfa /container-search/src | |
parent | a2b9e7ec76a39f31890fd854bbd43887e9507675 (diff) |
Preserve federation structure in Result on timeout
Diffstat (limited to 'container-search/src')
8 files changed, 55 insertions, 37 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(). |