Check for query cancellation during rewrite #53166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matriv
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
With ExitableDirectoryReader in place, check for query cancellation during QueryPhase#preProcess where the query rewriting takes place. Follows: elastic#52822
matriv
force-pushed
the
wire-cancellable-preprocess
branch
from
38a794e
to
f0d778f
Compare
|
@elasticmachine run elasticsearch-ci/2 |
jimczi
approved these changes
Mar 5, 2020
| @@ -110,7 +109,23 @@ public QueryPhase() { | |||
|
|
|||
| @Override | |||
| public void preProcess(SearchContext context) { | |||
| context.preProcess(true); | |||
| final Runnable cancellation; | |||
| if (context.lowLevelCancellation()) { | |||
There was a problem hiding this comment.
I think it's safe to ignore this parameter. Let's leave it for now but I wonder if we should simply remove this setting in a follow up. @jpountz what do you think ? I can open an issue if you think this deserves a full discussion.
There was a problem hiding this comment.
In the long term, I agree that we should remove it. The thing that isn't clear to me is how much it hurts terms-dict-intensive queries like fuzzy queries.
| final Runnable cancellation; | ||
| if (context.lowLevelCancellation()) { | ||
| cancellation = context.searcher().addQueryCancellation(() -> { | ||
| if (context.getTask().isCancelled()) { |
There was a problem hiding this comment.
let's get the task once outside of the runnable like we do below ?
| @@ -265,9 +280,8 @@ static boolean executeInternal(SearchContext searchContext) throws QueryPhaseExe | |||
| } | |||
|
|
|||
| if (searchContext.lowLevelCancellation()) { | |||
| SearchShardTask task = searchContext.getTask(); | |||
There was a problem hiding this comment.
I have a slight preference for this version. The task should never change during the execution of a search request.
matriv
added a commit
that referenced
this pull request
Mar 6, 2020
matriv
added a commit
to matriv/elasticsearch
that referenced
this pull request
Mar 23, 2020
Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 4095 docs. Moreover, set the cancellable task before calling QueryPhase.preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation() is set to false to avoid completely any performance impact. Follows: elastic#52822 Follows: elastic#53166 Follows: elastic#53496
matriv
added a commit
that referenced
this pull request
Mar 23, 2020
Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 8191 docs. Moreover, set the cancellable task before calling QueryPhase#preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation is set to false to avoid completely any performance impact. Follows: #52822 Follows: #53166 Follows: #53496
matriv
added a commit
that referenced
this pull request
Mar 23, 2020
Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 8191 docs. Moreover, set the cancellable task before calling QueryPhase#preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation is set to false to avoid completely any performance impact. Follows: #52822 Follows: #53166 Follows: #53496 (cherry picked from commit cdc377e)
This was referenced Apr 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With ExitableDirectoryReader in place, check for query cancellation
during QueryPhase#preProcess where the query rewriting takes place.
Follows: #52822