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

[Sessions] Extract search abort controllers logic into a separate class #95688

Merged

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Mar 29, 2021

Summary

Extract search abort controllers logic from the search interceptors into a separate class.

Needed for #92439 as it requires merging abort signals of multiple instances of cached requests.

Explanation

Aborting a cached search$ will now potentially depend on multiple abort signals (As described in #92439 (comment)).

To handle that, we will store the SearchAbortController in the cache, alongside the observable.

When a response will be returned from cache, we will use the addAbortSignal to make sure we abort the underlying search only when all consumers abort it.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom requested a review from a team as a code owner March 29, 2021 17:59
@lizozom lizozom self-assigned this Mar 29, 2021
@lizozom lizozom added v7.13.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:AppServices labels Mar 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

This approach makes sense to me... So in the search interceptor, if we get a cached observable, then we'd just addAbortSignal and return?

}
}

private abortHandler() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: So you don't have to do line 23

Suggested change
private abortHandler() {
private abortHandler = () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't understand this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Lukas pointed this out to avoid:
this.boundAbortHandler = this.abortHandler.bind(this);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM

Needed for #92439 as it requires merging abort signals of multiple instances of cached requests.

Could you elaborate, please? Maybe a short sample how it will be used will be helpful?

Will we store a searchAbortController instance as part of a cache?

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

looks good.
tested cancelations using Discover and Dashboard using slow network and shard delay
Didn't notice any user-facing changes

@lizozom lizozom added the Feature:Search Querying infrastructure in Kibana label Mar 30, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataEnhanced 97 98 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
data 202.8KB 202.8KB -1.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 804.1KB 802.8KB -1.3KB
dataEnhanced 23.9KB 25.4KB +1.5KB
kibanaUtils 144.1KB 143.1KB -1010.0B
total -836.0B

History

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

cc @lizozom

@lizozom lizozom merged commit b58dd3e into elastic:master Mar 30, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 30, 2021
…ss (elastic#95688)

* simplify abort controller logic and extract it into a class

* docs

* delete unused tests

* code review

* code review

* code review
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #95779

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 30, 2021
…ss (#95688) (#95779)

* simplify abort controller logic and extract it into a class

* docs

* delete unused tests

* code review

* code review

* code review

Co-authored-by: Liza Katz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants