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

Abort cancelled search requests to Elasticsearch #56788

Merged
merged 36 commits into from
Feb 25, 2020

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Feb 4, 2020

Summary

Resolves #55490.

Since the search service and KQL value suggestions were moved to the new platform, there had previously been no way in a route handler to listen for when a request was disconnected. This functionality has since been added. Following this PR, search requests (using the search service or KQL value suggestions) that initiate client-side that are disconnected will now close the upstream connection to Elasticsearch. This signals Elasticsearch to clean up any tasks associated with the originating search request.

Checklist

For maintainers

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 4, 2020
@lukasolson lukasolson requested a review from lizozom February 4, 2020 19:54
@lukasolson lukasolson requested review from a team as code owners February 4, 2020 19:54
@lukasolson lukasolson self-assigned this Feb 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

yarn.lock Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Feb 6, 2020
@lukasolson
Copy link
Member Author

@elasticmachine merge upstream

@lukasolson
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Code LGTM.

@@ -47,7 +47,7 @@ export const syncSearchStrategyProvider: TSearchStrategyProvider<typeof SYNC_SEA
signal: options.signal,
});

response.then(() => loadingCount$.next(loadingCount$.getValue() - 1));
response.finally(() => loadingCount$.next(loadingCount$.getValue() - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, if this is a bug fix and if there should be a test case for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is and yes there should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 159f5f0.

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM once green

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@lukasolson lukasolson merged commit 7b4c809 into elastic:master Feb 25, 2020
lukasolson added a commit to lukasolson/kibana that referenced this pull request Feb 25, 2020
* Update abort controller library

* Bootstrap

* Abort when the request is aborted

* Add utility and update value suggestions route

* Remove bad merge

* Revert switching abort controller libraries

* Revert package.json in lib

* Move to previous abort controller

* Fix test to use fake timers to run debounced handlers

* Fix loading bar not going away when cancelling

* Add test for loading count

* Fix test

* Fix failing test

Co-authored-by: Elastic Machine <[email protected]>
@lukasolson
Copy link
Member Author

7.x (7.7.0): b35f969

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 56788 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 26, 2020
@lukasolson lukasolson added v7.6.1 and removed v7.6.1 labels Feb 27, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 27, 2020
lukasolson added a commit to lukasolson/kibana that referenced this pull request Feb 28, 2020
* Update abort controller library

* Bootstrap

* Abort when the request is aborted

* Add utility and update value suggestions route

* Remove bad merge

* Revert switching abort controller libraries

* Revert package.json in lib

* Move to previous abort controller

* Fix test to use fake timers to run debounced handlers

* Fix loading bar not going away when cancelling

* Add test for loading count

* Fix test

* Fix failing test

Co-authored-by: Elastic Machine <[email protected]>
lukasolson added a commit that referenced this pull request Feb 28, 2020
* Abort cancelled search requests to Elasticsearch (#56788)

* Update abort controller library

* Bootstrap

* Abort when the request is aborted

* Add utility and update value suggestions route

* Remove bad merge

* Revert switching abort controller libraries

* Revert package.json in lib

* Move to previous abort controller

* Fix test to use fake timers to run debounced handlers

* Fix loading bar not going away when cancelling

* Add test for loading count

* Fix test

* Fix failing test

Co-authored-by: Elastic Machine <[email protected]>

* Remove unnecessary tests

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes review v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Search service] Integrate with aborted$ event to cancel discarded requests
7 participants