fix sqlab progress bar and status inconsistency #5848
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5848 +/- ##
==========================================
+ Coverage 63.75% 64.18% +0.43%
==========================================
Files 374 374
Lines 23320 23775 +455
Branches 2608 2624 +16
==========================================
+ Hits 14867 15261 +394
- Misses 8440 8501 +61
Partials 13 13
Continue to review full report at Codecov.
|
| @@ -200,7 +200,7 @@ export default class ResultSet extends React.PureComponent { | |||
| } | |||
| let progressBar; | |||
| let trackingUrl; | |||
| if (query.progress > 0 && query.state === 'running') { | |||
| if (query.progress > 0) { | |||
There was a problem hiding this comment.
Should we hide the progress bar after success?
There was a problem hiding this comment.
yeap. In the next round of rerendering SqlLab app, it will disappear. https://github.com/apache/incubator-superset/blob/cb92739c46355c3217b33297cbcce8801f5768aa/superset/assets/src/SqlLab/components/ResultSet.jsx#L165-#L189
| @@ -157,7 +157,7 @@ export const sqlLabReducer = function (state = {}, action) { | |||
| progress: 100, | |||
| results: action.results, | |||
| rows, | |||
| state: action.query.state, | |||
| state: 'rendering', | |||
There was a problem hiding this comment.
Is this where the code was setting state to success earlier?
There was a problem hiding this comment.
Nope. The state is still running and that's why the inconsistency between progress bar and status happens.
So after query succeeds, the action.results is returned from the server containing something like
{
'query': {
'id': ...,
'state': 'success',
'progress': 100,
...
}
'status': 'success'
}
but action.query itself is a part of the state of the app which contains {'state': 'running'} field in itself.
Then in the next round of rerendering, the query data returned from the server will show in the sqllab SouthPane and state will be updated in QueryAutoRefresh from running to success: https://github.com/apache/incubator-superset/blob/cb92739c46355c3217b33297cbcce8801f5768aa/superset/assets/src/SqlLab/components/QueryAutoRefresh.jsx#L45
So the inconsistency only exists during the period of rerendering, but users are still able to to see it and get confused.
Please correct me if my understanding is wrong.
There was a problem hiding this comment.
Got it. Thanks for explaining.
|
LGTM |
|
Often what takes time at 100% is the "push to s3" phase. Query is done, web server has the results, but it's pushing to s3. We may want to eventually introduce a new state for that. Happy to merge this as is for now as it's better than before. |
|
@kristw @timifasubaa @mistercrunch any clue on |
|
@youngyjd this is just travis hiccups, I just restarted the 2 jobs that hung up. |
|
@mistercrunch @kristw @timifasubaa tests passed. should be ready to go. |
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
) * revert 848 * nit (cherry picked from commit b1dbd1c)
mistercrunch
added
🏷️ bot
) * revert 848 * nit (cherry picked from commit eb92282)
This is an example that progress bar says 100% finished, but status still says running
This is because there is one more round of rendering going on in
sqllabapp to show data after it gets the query result.In this PR, I introduced one more state regarding query status which is
rendering. After query succeeds, the progress bar will now show100%and the status will showrendering. It is more user friendly. This is what it looks like now: