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

Fix aborting when searching without batching #49966

Merged
merged 4 commits into from
Nov 12, 2019

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Nov 1, 2019

Summary

#45235 included a regression for aborting requests. Since we were no longer assigning searching directly to the value of es.search, and instead also added a catch, the promise being returned didn't have the abort method that es.search returns.

This PR fixes that behavior so that requests are properly aborted, and adds tests to ensure this behavior is working properly.

Closes #49661

For testers

Requests should properly be aborted when issuing new requests or navigating away in Discover, Visualize, and Dashboard (other than the scenarios listed here: #46360

@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.6.0 labels Nov 1, 2019
@lukasolson lukasolson requested review from wylieconlon and a team November 1, 2019 18:01
@lukasolson lukasolson self-assigned this Nov 1, 2019
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@lukasolson

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@lukasolson

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@lukasolson
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukasolson lukasolson merged commit 573f990 into elastic:master Nov 12, 2019
@lukasolson
Copy link
Member Author

7.x (7.6.0): 0030dee

jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
* upstream/master:
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
…-fallback

* 'master' of github.com:elastic/kibana:
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 13, 2019
…ger-ace-theme

* 'master' of github.com:elastic/kibana: (56 commits)
  [ML] Server info service refactor (elastic#50302)
  Remove internal platform types exports (elastic#50427)
  [APM] Document `apm_oss.metricsIndices` and `apm_oss.sourcemap… (elastic#50312)
  [Telemetry] Server side fetcher (elastic#50015)
  [SIEM] Detection engine placeholders (elastic#50220)
  [Uptime] Donut chart loader position centered vertically  (elastic#50219)
  update telemetry banner notice text (elastic#50403)
  Fix aborting when searching without batching (elastic#49966)
  [Telemetry] Remove telemetry splash page and add conditional messaging (elastic#50189)
  Revert chromedriver update (elastic#50324)
  Remove deprecated argument include_type_name from ES calls (elastic#50285)
  [Maps] add settings to maps telemetry (elastic#50161)
  remove visualize loader (elastic#46910)
  Fix misuse of react-router and react-router-dom (elastic#50120)
  Move table-list-view to kibana-react (elastic#50046)
  [ML] Stats bar for data frame analytics (elastic#49464)
  [ML] Make navigation in tests more stable (elastic#50132)
  Migrate authorization subsystem to the new platform.  (elastic#46145)
  Bugfix: Interpreter conversion of string to number should throw on NaN elastic#27788 (elastic#50063)
  Update dependency @elastic/charts to v14 (elastic#49947)
  ...
rylnd pushed a commit to rylnd/kibana that referenced this pull request Nov 13, 2019
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Nov 14, 2019
@lukasolson lukasolson deleted the fixAbort branch December 2, 2019 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lens error message appears in the preview
3 participants