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

[sql lab] Fix setting async query status to timed_out #7429

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented May 2, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

In #4833, we tried to introduce a feature that setting old async query status to be timed_out, when the query status stuck at pending or running without updates after 6 hours. This status will help front-end (browser) not fetching very old query status. The bug i found is backend didn't commit transaction, so there is no records in query table has status timed_out (but keeps in running status for very very long time).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

When user's query has no updates after 6 hours, front-end should show this error message:
Screen Shot 2019-05-01 at 11 55 41 PM

TEST PLAN

send async query, then check query table. We should see old queries status that were running or pending, should updated to be timed_out.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@john-bodley @michellethomas @timifasubaa @mistercrunch

Sorry, something went wrong.

@graceguo-supercat graceguo-supercat force-pushed the gg-FixQueryTimedoutStatus branch from 7d4002c to 5883c22 Compare May 2, 2019 07:01
@codecov-io
Copy link

codecov-io commented May 2, 2019

Codecov Report

Merging #7429 into master will decrease coverage by <.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7429      +/-   ##
==========================================
- Coverage   65.26%   65.25%   -0.01%     
==========================================
  Files         430      430              
  Lines       21077    21080       +3     
  Branches     2338     2338              
==========================================
  Hits        13756    13756              
- Misses       7205     7208       +3     
  Partials      116      116
Impacted Files Coverage Δ
superset/views/core.py 72.39% <20%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 598526a...5883c22. Read the comment docs.

Query.client_id in queries_to_timeout,
),
).values(state=QueryStatus.TIMED_OUT)
for q in sql_queries:
Copy link
Member

@john-bodley john-bodley May 2, 2019

Choose a reason for hiding this comment

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

How many SQL queries are there? It may be more efficient to have the database (like before) handle this logic.

The issue here is that update(...) returns an expression which then needs to be executed and committed.

@graceguo-supercat
Copy link
Author

graceguo-supercat commented May 2, 2019

Hi @mistercrunch I have concern for this PR. We have 1.5M rows in query table since last June, the number is growing very fast. There may have over hundred ppl polling (and update) query status at the same time, for every second. At the same time, new queries are creating, and it also updates same table.

My concern is will this PR (and #4833) make query table too busy to handle, and finally cause table lock issue?

@graceguo-supercat
Copy link
Author

concern on the amount of polling requests, i think #7498 might be better.

@graceguo-supercat graceguo-supercat deleted the gg-FixQueryTimedoutStatus branch June 11, 2020 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants