Fix for polling queries

[betodealmeida]

CATEGORY

Choose one

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

SUMMARY

#7446 (review) changed the polling mechanism to keep polling when a query succeeds but doesn't have a resultsKey, to work around a race condition in SQL Lab. @graceguo-supercat pointed out that synchronous queries do not have resultsKey, and when they run the frontend keeps polling the backend unnecessarily.

This PR changes the logic so that the frontend keeps polling only for async queries that succeeded but don't have resultsKeys set.

TEST PLAN

Without this PR, this happens:

1. Run a sync query in SQL Lab, wait for results.
2. Create new tab, run async query.
3. Browser keeps polling after async query finished, since the sync query from (1) has no resultsKey.

With this PR, the browser stops polling once the async query finishes.

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

@graceguo-supercat

[codecov-io]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.18%. Comparing base (c79077d) to head (1a96cee).
⚠️ Report is 15099 commits behind head on master.

Files with missing lines	Patch %	Lines
.../assets/src/SqlLab/components/QueryAutoRefresh.jsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7559      +/-   ##
==========================================
- Coverage   65.20%   65.18%   -0.03%     
==========================================
  Files         433      433              
  Lines       21433    21428       -5     
  Branches     2360     2358       -2     
==========================================
- Hits        13975    13967       -8     
- Misses       7338     7341       +3     
  Partials      120      120              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mistercrunch]

Is there no way to make the state = success atomic along with the resultsKey affectation on the backend? The problem here is that success doesn't really mean "ok it's all done here!"

[betodealmeida]

Is there no way to make the state = success atomic along with the resultsKey affectation on the backend? The problem here is that success doesn't really mean "ok it's all done here!"

@mistercrunch in theory it should be. I fixed an eager commit that created a race condition, but even after deploying the fix @vylc reported it still happening. I was hoping it was a a bad node that didn't pick up the change, but I checked all hosts and they were indeed running the fix, which is why I made this workaround.

[mistercrunch]

Tried to look into the code as to where that commit may be happening and couldn't find it... Maybe something in there like the select_star method or something else triggers some sort of commit...

Would this help/work? The idea is that query.status = Query.SUCCESS doesn't need to be set up until the commit, so setting it right there, AFTER the query.results_key is set should logically prevent it to flip (assuming the issue is an uncontrolled commit hidden in the call stack)...
https://github.com/apache/incubator-superset/compare/master...mistercrunch:7559_alternative?expand=1

[betodealmeida]

@mistercrunch I think that would work. I still need to fix the polling though: currently it keeps polling when sync queries are present. Do you think we should keep this PR as is, since it's safer, or should I modify it to remove the line that says:

(q.state === 'success' && q.asyncRun && q.resultsKey === null)

[mistercrunch]

I think like we should remove that line that was there before, feels like duct tape to me. I opened the PR #7575 on my side.

[betodealmeida]

@mistercrunch @graceguo-supercat I reverted the changes that made the unnecessary polling.

[graceguo-supercat]

thanks @betodealmeida

Look at this line, where we set query status Success and results_key:
https://github.com/apache/incubator-superset/blob/5ef2712e16e07b394cad809d28e1dcb65a48c97f/superset/sql_lab.py#L272

we set status at line 272, while we set results_key at line 299. Do you think lines between this two will be executed atomic? otherwise we will have some query has status but no results_key.

[betodealmeida]

@graceguo-supercat yeah, that might be the problem. @mistercrunch moved that line (setting the status to success) to before the commit, ensuring that it has results_key (see #7575).

[graceguo-supercat]

wow thanks!! the issue is finally got fixed!

[graceguo-supercat]

LGTM