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

Show clear and actionable query timeout error message #2763

Merged
merged 3 commits into from
May 16, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* global notify */
import React from 'react';
import PropTypes from 'prop-types';
import { FormControl, FormGroup, Overlay, Popover, Row, Col } from 'react-bootstrap';
Expand Down
4 changes: 2 additions & 2 deletions superset/assets/javascripts/SqlLab/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import thunkMiddleware from 'redux-thunk';

import { getInitialState, sqlLabReducer } from './reducers';
import { initEnhancer } from '../reduxUtils';
import { initJQueryAjaxCSRF } from '../modules/utils';
import { initJQueryAjax } from '../modules/utils';
import App from './components/App';
import { appSetup } from '../common';

Expand All @@ -15,7 +15,7 @@ import './reactable-pagination.css';
import '../components/FilterableTable/FilterableTableStyles.css';

appSetup();
initJQueryAjaxCSRF();
initJQueryAjax();

const appContainer = document.getElementById('app');
const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap'));
Expand Down
1 change: 1 addition & 0 deletions superset/assets/javascripts/constants.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const QUERY_TIMEOUT_THRESHOLD = 60000;
2 changes: 1 addition & 1 deletion superset/assets/javascripts/dashboard/Dashboard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ export function dashboardContainer(dashboard, datasources, userid) {

$(document).ready(() => {
// Getting bootstrapped data from the DOM
utils.initJQueryAjaxCSRF();
utils.initJQueryAjax();
const dashboardData = $('.dashboard').data('bootstrap');

const state = getInitialState(dashboardData);
Expand Down
26 changes: 20 additions & 6 deletions superset/assets/javascripts/explorev2/actions/exploreActions.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint camelcase: 0 */
import { getExploreUrl } from '../exploreUtils';
import { QUERY_TIMEOUT_THRESHOLD } from '../../constants';

const $ = window.$ = require('jquery');

Expand Down Expand Up @@ -150,6 +151,11 @@ export function chartUpdateStopped(queryRequest) {
return { type: CHART_UPDATE_STOPPED };
}

export const CHART_UPDATE_TIMEOUT = 'CHART_UPDATE_TIMEOUT';
export function chartUpdateTimeout(statusText) {
return { type: CHART_UPDATE_TIMEOUT, statusText };
}

export const CHART_UPDATE_FAILED = 'CHART_UPDATE_FAILED';
export function chartUpdateFailed(queryResponse) {
return { type: CHART_UPDATE_FAILED, queryResponse };
Expand Down Expand Up @@ -234,12 +240,20 @@ export const RUN_QUERY = 'RUN_QUERY';
export function runQuery(formData, force = false) {
return function (dispatch) {
const url = getExploreUrl(formData, 'json', force);
const queryRequest = $.getJSON(url, function (queryResponse) {
dispatch(chartUpdateSucceeded(queryResponse));
}).fail(function (err) {
if (err.statusText !== 'abort') {
dispatch(chartUpdateFailed(err.responseJSON));
}
const queryRequest = $.ajax({
url,
dataType: 'json',
success(queryResponse) {
dispatch(chartUpdateSucceeded(queryResponse));
},
error(err) {
if (err.statusText === 'timeout') {
dispatch(chartUpdateTimeout(err.statusText));
} else if (err.statusText !== 'abort') {
dispatch(chartUpdateFailed(err.responseJSON));
}
},
timeout: QUERY_TIMEOUT_THRESHOLD,
});
dispatch(chartUpdateStarted(queryRequest));
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,14 @@ class ChartContainer extends React.PureComponent {
renderAlert() {
const msg = (
<div>
{this.props.alert}
<i
className="fa fa-close pull-right"
onClick={this.removeAlert.bind(this)}
style={{ cursor: 'pointer' }}
/>
<p
dangerouslySetInnerHTML={{ __html: this.props.alert }}
/>
</div>);
return (
<div>
Expand Down
4 changes: 2 additions & 2 deletions superset/assets/javascripts/explorev2/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ import { now } from '../modules/dates';
import { initEnhancer } from '../reduxUtils';
import AlertsWrapper from '../components/AlertsWrapper';
import { getControlsState, getFormDataFromControls } from './stores/store';
import { initJQueryAjaxCSRF } from '../modules/utils';
import { initJQueryAjax } from '../modules/utils';
import ExploreViewContainer from './components/ExploreViewContainer';
import { exploreReducer } from './reducers/exploreReducer';
import { appSetup } from '../common';
import './main.css';

appSetup();
initJQueryAjaxCSRF();
initJQueryAjax();

const exploreViewContainer = document.getElementById('js-explore-view-container');
const bootstrapData = JSON.parse(exploreViewContainer.getAttribute('data-bootstrap'));
Expand Down
11 changes: 11 additions & 0 deletions superset/assets/javascripts/explorev2/reducers/exploreReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { getControlsState, getFormDataFromControls } from '../stores/store';
import * as actions from '../actions/exploreActions';
import { now } from '../../modules/dates';
import { QUERY_TIMEOUT_THRESHOLD } from '../../constants';

export const exploreReducer = function (state, action) {
const actionHandlers = {
Expand Down Expand Up @@ -109,6 +110,16 @@ export const exploreReducer = function (state, action) {
triggerQuery: true,
});
},
[actions.CHART_UPDATE_TIMEOUT]() {
return Object.assign({}, state, {
chartStatus: 'failed',
chartAlert: '<strong>Query timeout</strong> - visualization query are set to timeout at ' +
`${QUERY_TIMEOUT_THRESHOLD / 1000} seconds. ` +
'Perhaps your data has grown, your database is under unusual load, ' +
'or you are simply querying a data source that is to large to be processed within the timeout range. ' +
'If that is the case, we recommend that you summarize your data further.',
});
},
[actions.CHART_UPDATE_FAILED]() {
return Object.assign({}, state, {
chartStatus: 'failed',
Expand Down
2 changes: 1 addition & 1 deletion superset/assets/javascripts/modules/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export function customizeToolTip(chart, xAxisFormatter, yAxisFormatters) {
});
}

export function initJQueryAjaxCSRF() {
export function initJQueryAjax() {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why the function name is changed in this PR?

Copy link
Author

Choose a reason for hiding this comment

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

there are actually multiple places need to handle 504 Gateway timeout problem for long query (but this ticket only fix for explore view).

At the beginning I want to set a default jQuery ajax timeout limit from ajaxSetup, for all dashboard and explore view queries. It seems currently we only used this setup for CSRF, so it named as initJQueryAjaxCSRF. I am just thinking we might need a function to setup global, default jQuery ajax related settings, and this function is already hooked-up by many components.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense!

// Works in conjunction with a Flask-WTF token as described here:
// http://flask-wtf.readthedocs.io/en/stable/csrf.html#javascript-requests
const token = $('input#csrf_token').val();
Expand Down
21 changes: 21 additions & 0 deletions superset/assets/spec/javascripts/explorev2/actions_spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { it, describe } from 'mocha';
import { expect } from 'chai';
import sinon from 'sinon';
import $ from 'jquery';
import * as actions from '../../../javascripts/explorev2/actions/exploreActions';
import * as exploreUtils from '../../../javascripts/explorev2/exploreUtils';
import { defaultState } from '../../../javascripts/explorev2/stores/store';
import { exploreReducer } from '../../../javascripts/explorev2/reducers/exploreReducer';

Expand All @@ -16,3 +19,21 @@ describe('reducers', () => {
expect(newState.controls.show_legend.value).to.equal(true);
});
});

describe('runQuery', () => {
it('should handle query timeout', () => {
const dispatch = sinon.spy();
const urlStub = sinon.stub(exploreUtils, 'getExploreUrl', () => ('mockURL'));
const ajaxStub = sinon.stub($, 'ajax');
ajaxStub.yieldsTo('error', { statusText: 'timeout' });

const request = actions.runQuery({});
request(dispatch);

expect(dispatch.callCount).to.equal(2);
expect(dispatch.args[0][0].type).to.equal(actions.CHART_UPDATE_TIMEOUT);

urlStub.restore();
ajaxStub.restore();
});
});