Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for query cancellation during rewrite #53166

Merged
merged 5 commits into from
Mar 5, 2020

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Mar 5, 2020

With ExitableDirectoryReader in place, check for query cancellation
during QueryPhase#preProcess where the query rewriting takes place.

Follows: #52822

@matriv matriv added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.7.0 labels Mar 5, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@matriv matriv requested review from jimczi and jpountz and removed request for jimczi March 5, 2020 13:55
With ExitableDirectoryReader in place, check for query cancellation
during QueryPhase#preProcess where the query rewriting takes place.

Follows: elastic#52822
@matriv matriv force-pushed the wire-cancellable-preprocess branch from 38a794e to f0d778f Compare March 5, 2020 14:02
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me but please wait for @jimczi to have a look.

@matriv
Copy link
Contributor Author

matriv commented Mar 5, 2020

@elasticmachine run elasticsearch-ci/2

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM2

@@ -110,7 +109,23 @@ public QueryPhase() {

@Override
public void preProcess(SearchContext context) {
context.preProcess(true);
final Runnable cancellation;
if (context.lowLevelCancellation()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for this version. The task should never change during the execution of a search request.

@matriv matriv merged commit 0d38626 into elastic:master Mar 5, 2020
@matriv matriv deleted the wire-cancellable-preprocess branch March 5, 2020 23:19
matriv added a commit that referenced this pull request Mar 6, 2020
With ExitableDirectoryReader in place, check for query cancellation
during QueryPhase#preProcess where the query rewriting takes place.

Follows: #52822

(cherry picked from commit 0d38626)
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)
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants