Skip to content

Remove courier and search poller and move to time filter service#48301

Merged
lukasolson merged 2 commits intoelastic:masterfrom
lukasolson:removeSearchPoller
Oct 17, 2019
Merged

Remove courier and search poller and move to time filter service#48301
lukasolson merged 2 commits intoelastic:masterfrom
lukasolson:removeSearchPoller

Conversation

@lukasolson
Copy link
Copy Markdown
Contributor

@lukasolson lukasolson commented Oct 15, 2019 •
edited
Loading

Summary

This PR removes the search poller and courier Angular services and moves the associated auto-refresh logic directly into the time filter service.

This means that applications no longer need to require or rely on the courier service for enabling auto-refresh capabilities.

Checklist

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

For testers

This PR shouldn't have any functionality changes to test, but I've tried to focus on testing all functionality related to auto-refresh.

@lukasolson lukasolson requested review from a team and lizozom October 15, 2019 22:01
@lukasolson lukasolson requested a review from a team as a code owner October 15, 2019 22:01
@lukasolson lukasolson self-assigned this Oct 15, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 15, 2019

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

@lukasolson lukasolson added the release_note:skip Skip the PR/issue when compiling release notes label Oct 15, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 15, 2019

💚 Build Succeeded

lizozom
lizozom reviewed Oct 16, 2019
View reviewed changes
src/legacy/core_plugins/data/public/timefilter/timefilter.ts Outdated
Copy link
Copy Markdown
Contributor

@lizozom lizozom Oct 16, 2019

Choose a reason for hiding this comment

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

Can you please remove this method and just trigger the observable from setRefreshInterval?

Copy link
Copy Markdown
Contributor Author

@lukasolson lukasolson Oct 16, 2019

Choose a reason for hiding this comment

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

I could remove it, yes, but I'd still need to declare it as a TimerHandler otherwise Typescript complains.

Copy link
Copy Markdown
Contributor Author

@lukasolson lukasolson Oct 16, 2019

Choose a reason for hiding this comment

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

Turns out if I added window.setInterval then Typescript appropriately associates the types.

lizozom
lizozom suggested changes Oct 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lizozom lizozom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for making this PR! I was planning to do this in the near future.

I Tested this in multiple applications and there are quite a few oddities (I'm not sure which of them are a result of this PR, and which are are already in master).

  • Data Visualizations (tested TSVB and gauge), Dashboard and APM - don't fetch data when you start auto refresh (only after 1 interval)
  • Discover - fetches data immediately 2 times when you start auto refresh
  • Maps seems to fetch data twice every time auto refresh update is triggered

walterra
walterra approved these changes Oct 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML changes LGTM (I didn't see any double-loading with the ML and transform plugin during refreshs).

@lukasolson
Copy link
Copy Markdown
Contributor Author

lukasolson commented Oct 16, 2019

@lizozom

Data Visualizations (tested TSVB and gauge), Dashboard and APM - don't fetch data when you start auto refresh (only after 1 interval)

Confirmed happening on master as well.

Discover - fetches data immediately 2 times when you start auto refresh

Confirmed happening on master as well.

Maps seems to fetch data twice every time auto refresh update is triggered

I couldn't reproduce this, are you talking about the maps app or a region/coordinate map? If the app, then do you have multiple layers causing separate requests?

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 17, 2019

💚 Build Succeeded

lizozom
lizozom approved these changes Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
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

@lukasolson lukasolson merged commit e20457e into elastic:master Oct 17, 2019
lukasolson added a commit to lukasolson/kibana that referenced this pull request Oct 17, 2019
@lukasolson
Remove courier and search poller and move to time filter service (ela…
eca81ec 
…stic#48301)

* Remove courier and search poller and move to time filter service

* Remove notifyShouldFetch function
@lukasolson
Copy link
Copy Markdown
Contributor Author

lukasolson commented Oct 17, 2019

7.x (7.6.0): eca81ec

@lukasolson lukasolson deleted the removeSearchPoller branch December 2, 2019 22:36
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 11, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
3d2ee5f 
…stic#59781 and elastic#48301
@dokmic dokmic mentioned this pull request Jul 14, 2025
Closed
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 14, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
a49eaf2 
…stic#59781 and elastic#48301 (elastic#227776)
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 14, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
f327a53 
…stic#59781 and elastic#48301 (elastic#227776)
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 14, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
fe6b95a 
…stic#59781 and elastic#48301 (elastic#227776)
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 14, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
f83c395 
…stic#59781 and elastic#48301 (elastic#227776)
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 17, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
33c1bdd 
…stic#59781 and elastic#48301 (elastic#227776)
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 24, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
1ba39de 
…stic#59781 and elastic#48301 (elastic#227776)
dokmic added a commit to dokmic/kibana that referenced this pull request Jul 28, 2025
@dokmic
Remove code returning fatal error info that became obsolete after ela…
1aaf297 
…stic#59781 and elastic#48301 (elastic#227776)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@walterra walterra walterra approved these changes

+1 more reviewer

@lizozom lizozom lizozom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukasolson lukasolson

Labels

Feature:Search Querying infrastructure in Kibana Feature:Timepicker Timepicker release_note:skip Skip the PR/issue when compiling release notes review v7.6.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lukasolson @elasticmachine @walterra @lizozom