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

Cancel visualize fetches when navigating away or re-fetching #42035

Merged
merged 33 commits into from
Aug 28, 2019

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Jul 25, 2019

Summary

Depends on #40238.

Fixes #17844.
Fixes #17845.
Fixes #17846.
Fixes #24058.

This PR updates visualize embeddables to cancel existing fetch requests when a new fetch request is submitted. This can be when something is changed (like filters, time range, query, agg config, etc.), when clicking Refresh, or when navigating away.

Release Notes

In-progress search requests to Elasticsearch will now be cancelled when a new search is requested, or when a user navigates away from the page or removes a panel from a dashboard. In order for Elasticsearch to cancel the tasks associated with search requests that are cancelled, please disable the advanced setting courier:batchSearches (which is enabled by default).

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana Team:AppArch labels Jul 25, 2019
@lukasolson lukasolson requested a review from a team as a code owner July 25, 2019 23:54
@lukasolson lukasolson self-assigned this Jul 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukasolson
Copy link
Member Author

@ppisljar Could you take another look at this? I think we can push this over the line before we do a big refactor of the expressions service.

@@ -435,7 +436,13 @@ export class EmbeddedVisualizeHandler {
this.fetchAndRender();
};

private cancel = () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be public?

Copy link
Member

Choose a reason for hiding this comment

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

yup, i think so, but not necessary for now
we'll want to cancel externally (once we have cancel buttons around)

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

few minor comments, haven't tested yet

@@ -435,7 +436,13 @@ export class EmbeddedVisualizeHandler {
this.fetchAndRender();
};

private cancel = () => {
Copy link
Member

Choose a reason for hiding this comment

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

yup, i think so, but not necessary for now
we'll want to cancel externally (once we have cancel buttons around)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukasolson
Copy link
Member Author

Retest

@lukasolson
Copy link
Member Author

@ppisljar Maybe we can jump on a zoom tomorrow to see what's happening. It seems to be aborting just fine for me, when interpreter.enableInVisualize is set to both true and false.

@lukasolson
Copy link
Member Author

lukasolson commented Aug 21, 2019

Took another look at this today and realize there are some cases where we aren't successfully cancelling fetches:

  • Vega visualizations
  • TSVB visualizations
  • Input controls
  • Saved searches (which is what is popping up the "Abort was requested" errors @ppisljar was referring to)
  • Maps embeddables

With regards to saved searches, it should be easy to update them to cancel once #43634 is merged.

With regards to the others, I think it'd be best to open a follow-up PR to handle these cases.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukasolson
Copy link
Member Author

@ppisljar Could you take another look at this?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

postFlightRequests (like other bucket for term agg) are not being canceled

@@ -76,13 +76,13 @@ export const histogramBucketAgg = new BucketAggType({
{
name: 'interval',
editorComponent: NumberIntervalParamEditor,
modifyAggConfigOnSearchRequestStart(aggConfig, searchSource) {
modifyAggConfigOnSearchRequestStart(aggConfig, searchSource, searchRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

can we rather pass abortSignal in here to have same implementation as in other places ?( abortSignal.addEventListener)

Copy link
Member Author

@lukasolson lukasolson Aug 28, 2019

Choose a reason for hiding this comment

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

This won't quite work since this method is called from both buildVislibDimensions (which has access to the abortSignal but not the searchRequest) and searchSource.requestIsStarting (which has access to the searchRequest but not the abortSignal). I could theoretically pass in abortSignal as an additional argument, then check both searchRequest and abortSignal but that seems kinda messy. Any other proposals?

@spalger
Copy link
Contributor

spalger commented Aug 28, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukasolson lukasolson merged commit 9fecf8d into elastic:master Aug 28, 2019
lukasolson added a commit to lukasolson/kibana that referenced this pull request Aug 28, 2019
…#42035)

* add AbortSignal to interpreter

* adding AbortSignal to visualize_loader

* adding AbortSignal to embeddables and dashboard

* passing AbortSignal to courier request handler

* Remove abort signal from dashboard and move to handler, and abort fetches when necessary

* Remove the rest of the references to abort signal in dashboard

* Revert changes to dashboard_app

* Remove code related to canceling visualize requests and only keep stuff for canceling interpreter

* Use createError

* Cancel in-progress fetches when a new one is requested or when leaving the page

* Update with cancel methods and make visualize data loader work

* Remove unnecessary call to

* Fix tests

* Remove cancel from data loaders

* Fix search source to always either resolve or reject in fetch()

* Fix naming

* Update search request to reject on abort, and don't render errors in the UI

* Update tests

* Add cancellation to input control

* Cancel histogram fetches

* Update esaggs typings

* Add cancellation for search embeddables

* Abort post-flight requests
@lukasolson
Copy link
Member Author

7.x (7.4.0): d7a7c73

@lukasolson lukasolson deleted the cancelVisualizeRequests branch December 2, 2019 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment