Skip to content

[Maps] cancel SearchSource fetches that are no longer needed.#44436

Merged
nreese merged 13 commits intoelastic:masterfrom
nreese:cancel_fetch
Sep 19, 2019
Merged

[Maps] cancel SearchSource fetches that are no longer needed.#44436
nreese merged 13 commits intoelastic:masterfrom
nreese:cancel_fetch

Conversation

@nreese
Copy link
Contributor

@nreese nreese commented Aug 29, 2019

SearchSource and courier now contain the infrastructure needed to cancel in-flight _search results. This is great because there are lots of actions that will negate the need for an in-flight SearchSource fetch. These include panning/zooming the map to new areas, changing query context, removing layers, and leaving a map. The Maps application needs to cancel in-flight SearchSource fetches that are no longer needed to avoid causing pressure on Elasticsearch nodes.

@nreese nreese added release_note:enhancement WIP Work in progress Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.5.0 labels Aug 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese removed the WIP Work in progress label Sep 3, 2019
@nreese nreese requested review from kindsun and lukasolson September 3, 2019 20:46
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@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.

Sep-04-2019 14-06-27

Hmm, when I remove the map from a dashboard, it seems like it cancels one of the requests, but additional _msearch requests keep on getting queued up (and running).

@kindsun
Copy link
Contributor

kindsun commented Sep 5, 2019

I'm getting the following console error when doing a normal filter by bounds:

image

I was using the ecommerce sample data map.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Sep 9, 2019

I'm getting the following console error when doing a normal filter by bounds

That error is displayed by courier when requests are canceled and outside the scope of this PR. Issue tracked in issue #44738

@nreese nreese requested a review from kindsun September 9, 2019 18:22
@nreese
Copy link
Contributor Author

nreese commented Sep 9, 2019

Hmm, when I remove the map from a dashboard, it seems like it cancels one of the requests, but additional _msearch requests keep on getting queued up (and running)

@lukasolson This behaviour is the result of a bug in map embeddable and the component not getting cleaned up. I opened #45183 to fix the bug and will ping you for another review once that PR is merged.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Sep 10, 2019

@lukasolson #45183 is merged and this PR is ready for review

@nreese nreese requested a review from lukasolson September 10, 2019 11:37
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@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.

LGTM, tested in Chrome and seems to be properly aborting requests. One minor thing I noticed is that sometimes if you have multiple layers and hit refresh several times, then some of the layers display a warning symbol until it loads. We might want to add special handling of aborted request errors there:

Kapture 2019-09-12 at 14 26 57

@nreese
Copy link
Contributor Author

nreese commented Sep 16, 2019

One minor thing I noticed is that sometimes if you have multiple layers and hit refresh several times, then some of the layers display a warning symbol until it loads. We might want to add special handling of aborted request errors there

Thanks for reviewing this PR so thoroughly and finding this behavior. This is caused because the current implementation shallows AbortErrors to early and returns a response of undefined.

I have updated the PR to bubble up the DataRequestAbortError when requests are aborted so the caller can properly handle aborted requests.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese requested a review from kindsun September 19, 2019 20:55
Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

Thanks for moving the callback tracking to the store. Much nicer. lgtm!

  • code review
  • tested locally

@nreese nreese merged commit 6f62670 into elastic:master Sep 19, 2019
nreese added a commit to nreese/kibana that referenced this pull request Sep 19, 2019
…c#44436)

* [Maps] cancel SearchSource fetch on new request and destroy

* cancel requests on layer delete and map unmount

* use delete function to unregister cancel callback

* review feedback

* move cancelRequest to action

* bubble request abort error up to caller
nreese added a commit that referenced this pull request Sep 20, 2019
#46199)

* [Maps] cancel SearchSource fetch on new request and destroy

* cancel requests on layer delete and map unmount

* use delete function to unregister cancel callback

* review feedback

* move cancelRequest to action

* bubble request abort error up to caller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:enhancement Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.5.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants