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 previous execution on parameters value update #3850

Closed
wants to merge 1 commit into from

Conversation

gabrieldutra
Copy link
Member

What type of PR is this? (check all applicable)

  • Other

Description

Hi 🙂

I was a bit concerned about slow queries after #3737 (when updating many parameters at once). One thing that can improve this a lot (@ranbena mentioned in the PR) is to cancel the previous execution before running the new one. This idea was simple enough to open a PR directly for discussion from it. The concerns I have regarding it:

  • It's necessary that the cancel operation works properly. If it doesn't, the behavior will be no different than it is now. I remember there were issues with MySQL cancelling (afaik a recent PR fixed that), but for PostgreSQL it works nicely.
  • Can there be any performance issues with it, like, is it a function that is not supposed to be used that often?
  • Are there datasources without cancel operation?

I guess this can be also done backend-wise, but I opted to update it as is to change it only where it could be needed (on parameters value update).

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

--

@gabrieldutra gabrieldutra self-assigned this May 29, 2019
@arikfr
Copy link
Member

arikfr commented May 30, 2019

It's necessary that the cancel operation works properly. If it doesn't, the behavior will be no different than it is now.

This is exactly my concern here. I'm not entirely sure about the situation with cancel, but I do know some people reported it causing some trouble. While some of it might be related to the issue that was fixed with MySQL, it's possible there are other data sources that have this issue.

Also, with some data sources the cancel just cancels the Redash job, but not the query execution in the database.

For these reasons, I prefer to try and minimize unneeded executions instead of cancelling them -> https://discuss.redash.io/t/what-to-do-when-you-want-to-update-several-parameters-before-running-a-query/3906

Let's keep this PR around for now, but wait with following up with it until we explore other ideas.

@gabrieldutra
Copy link
Member Author

Closing in favor of #3907 🙂

@gabrieldutra gabrieldutra deleted the parameters-cancel-previous-run branch January 13, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants