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

[TSVB] Old request might override newer request_handler #34541

Closed
wants to merge 6 commits into from

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Apr 4, 2019

Fix: #14208

Summary

Description of the problem including expected versus actual behavior:
TSVB doesn't abort old queries when new ones are created

Steps to reproduce:

  1. Create a new TSVB time-series visualization
  2. Run a long query (over a long time range)
  3. While the previous query is still running, update the time range to smaller range
  4. The new query will finish before the first one, and update the visualization, and then the old query will finish and override the visualization, even though it was basically "aborted"

Also you can reproduce this issue using Network Throttling Profile:

perf_data

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine Apr 5, 2019
@elastic elastic deleted a comment from elasticmachine Apr 5, 2019
@elastic elastic deleted a comment from elasticmachine Apr 5, 2019
@elastic elastic deleted a comment from elasticmachine Apr 5, 2019
@alexwizp alexwizp requested a review from spalger April 5, 2019 13:04
@alexwizp alexwizp added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix labels Apr 5, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@alexwizp alexwizp marked this pull request as ready for review April 8, 2019 14:10
@markov00 markov00 self-assigned this Apr 8, 2019
@markov00
Copy link
Member

markov00 commented Apr 9, 2019

Hey @elastic/kibana-app-arch can somebody review this PR because it includes a change to the vis loader. In particular, Alex is adding an visInstanceId to the VisualizeLoaderParams, this is used to have a unique ID of the instantiated visualization on a dashboard so that they can abort an old fetch call if a subsequent request is made. The ID is needed because we can have multiple instances of the same visualization on a single dashboard.

@markov00 markov00 requested a review from a team April 9, 2019 09:05
@alexwizp alexwizp requested a review from markov00 April 10, 2019 14:16
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alexwizp alexwizp requested a review from ppisljar April 12, 2019 12:16
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.

I see a few problems with this implementation:

  • solves issue for tsvb only, seems as a general problem with our infrastructure which will affect everything
  • we shouldn't need visInstanceId, each panel on dashboard has its own instance of visualize_embeddable_handler, which should take care of canceling old request
  • having visInstanceId as parameter, what happens when we give users ability to type query (like in canvas) ? having to type this in doesn't seem optimal.

i think a better approach would be to solve this on the embedded_visualize_handler level:

  • in update, see if previous fetch/render is still in progress and cancel it.

For now fetch execution wouldn't be canceled, but we would just ignore the old request, as courier doesn't support query cancellation at this point, nor does interpreter. But this would work well with our plans for the future:

  • with @timroes we discussed a while back that we will need a general way to cancel interpreter expression execution, where every function could provide a cancel method.
  • lucas is working on courier and adding query canceling support

@lukeelmers
Copy link
Member

I agree with @ppisljar's comments here; since this is a more general problem we shouldn't do something TSVB-specific.

The issue is also already captured in #24058 -- I'm wondering if we should just close #14208 as a duplicate.

@alexwizp
Copy link
Contributor Author

@lukeelmers @ppisljar I'll close this PR and will wait for common solution

@alexwizp alexwizp closed this Apr 16, 2019
@alexwizp alexwizp deleted the 14208 branch January 4, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Old request might override newer request
5 participants