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.

Sign up for GitHub

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

fix sqlab progress bar and status inconsistency #5848

Merged
merged 1 commit into from
Sep 20, 2018
Merged

fix sqlab progress bar and status inconsistency #5848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset/assets/src/SqlLab/components/QueryAutoRefresh.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class QueryAutoRefresh extends React.PureComponent {
const now = new Date().getTime();
return Object.values(queries)
.some(
q => ['running', 'started', 'pending', 'fetching'].indexOf(q.state) >= 0 &&
q => ['running', 'started', 'pending', 'fetching', 'rendering'].indexOf(q.state) >= 0 &&
now - q.startDttm < MAX_QUERY_AGE_TO_POLL,
);
}
Expand Down
2 changes: 1 addition & 1 deletion superset/assets/src/SqlLab/components/ResultSet.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we hide the progress bar after success?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

progressBar = (
<ProgressBar
striped
Expand Down
1 change: 1 addition & 0 deletions superset/assets/src/SqlLab/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export const STATE_BSSTYLE_MAP = {
fetching: 'info',
running: 'warning',
stopped: 'danger',
rendering: 'info',
success: 'success',
};

Expand Down
2 changes: 1 addition & 1 deletion superset/assets/src/SqlLab/reducers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export const sqlLabReducer = function (state = {}, action) {
progress: 100,
results: action.results,
rows,
state: action.query.state,
state: 'rendering',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this where the code was setting state to success earlier?

Copy link
Contributor Author

@youngyjd youngyjd Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks for explaining.

errorMessage: null,
cached: false,
};
Expand Down