[Sqllab] Add offline state to sqllab #6013

timifasubaa · 2018-10-01T07:56:14Z

This is a user experience improvement in sqllab. Historically, if the user was disconnected from the internet or the backed was down, sqllab continues to poll from the backend. All the while showing the user they are in the pending state. This is confusing to the user as they would keep thinking their query is pending for a long time when the request hasn't been sent to the backend.
This PR makes it show offline when the ajax request cannot connect to the backend to avoid confusion .

@graceguo-supercat @kristw

codecov-io · 2018-10-01T19:04:17Z

Codecov Report

Merging #6013 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6013   +/-   ##
=======================================
  Coverage   76.91%   76.91%           
=======================================
  Files          47       47           
  Lines        9362     9362           
=======================================
  Hits         7201     7201           
  Misses       2161     2161

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 a71e6eb...3879751. Read the comment docs.

graceguo-supercat · 2018-10-01T21:33:18Z

superset/assets/src/SqlLab/components/QueryAutoRefresh.jsx

+      $.ajax({
+        dataType: 'json',
+        url,
+        timeout: 3000,


this line will make all long query (>3 seconds) abort.

Okay, I will remove the timeout option.

graceguo-supercat · 2018-10-01T21:34:04Z

superset/assets/src/SqlLab/components/QueryAutoRefresh.jsx

+        },
+        error: (XMLHttpRequest) => {
+          if (XMLHttpRequest.readyState === 0) {
+            document.location.reload(true);


i will prefer show error message not force refresh user's browser.

timifasubaa · 2018-10-11T08:51:09Z

Ready for review

graceguo-supercat · 2018-10-04T18:51:01Z

superset/assets/src/SqlLab/components/QueryAutoRefresh.jsx

@@ -38,12 +38,22 @@ class QueryAutoRefresh extends React.PureComponent {
  }
  stopwatch() {
    // only poll /superset/queries/ if there are started or running queries
+    const self = this;


you don't need self variable. see the exist code.

also corrected.

graceguo-supercat · 2018-10-09T18:37:13Z

superset/assets/src/SqlLab/App.jsx

@@ -20,6 +20,7 @@ initJQueryAjax();

 const appContainer = document.getElementById('app');
 const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap'));
+bootstrapData.offline = false;


this logic (set initial value for offline state) should be in function getInitialState

graceguo-supercat · 2018-10-11T18:22:05Z

superset/assets/src/SqlLab/components/TabbedSqlEditors.jsx

@@ -21,9 +21,11 @@ const propTypes = {
  tabHistory: PropTypes.array.isRequired,
  tables: PropTypes.array.isRequired,
  getHeight: PropTypes.func.isRequired,
+  offline: PropTypes.func


what is offline prop type?

Oops. Corrected.

graceguo-supercat · 2018-10-11T18:35:10Z

superset/assets/src/SqlLab/components/SqlEditorLeftBar.jsx

 SqlEditorLeftBar.propTypes = propTypes;
 SqlEditorLeftBar.defaultProps = defaultProps;

-export default SqlEditorLeftBar;
+export default connect(mapStateToProps)(SqlEditorLeftBar);


is adding redux connect for this component necessary? where do you need to check state?

it's necessary because I ensure it is online before the ajax calls

timifasubaa · 2018-10-12T00:40:45Z

All concerns addressed.

kristw · 2018-10-12T00:52:30Z

superset/assets/src/SqlLab/actions.js

@@ -34,6 +34,7 @@ export const SET_DATABASES = 'SET_DATABASES';
 export const SET_ACTIVE_QUERY_EDITOR = 'SET_ACTIVE_QUERY_EDITOR';
 export const SET_ACTIVE_SOUTHPANE_TAB = 'SET_ACTIVE_SOUTHPANE_TAB';
 export const REFRESH_QUERIES = 'REFRESH_QUERIES';
+export const USER_OFFLINE = 'USER_OFFLINE';


Rename this to SET_USER_OFFLINE to make verb like other action names.

very good catch!

kristw · 2018-10-12T00:52:54Z

superset/assets/src/SqlLab/actions.js

@@ -373,6 +374,10 @@ export function refreshQueries(alteredQueries) {
  return { type: REFRESH_QUERIES, alteredQueries };
 }

+export function userOffline(offline) {


userOffline => setUserOffline

superset/assets/spec/javascripts/sqllab/SqlEditorLeftBar_spec.jsx

graceguo-supercat · 2018-10-12T05:00:21Z

superset/assets/src/SqlLab/actions.js

@@ -34,6 +34,7 @@ export const SET_DATABASES = 'SET_DATABASES';
 export const SET_ACTIVE_QUERY_EDITOR = 'SET_ACTIVE_QUERY_EDITOR';
 export const SET_ACTIVE_SOUTHPANE_TAB = 'SET_ACTIVE_SOUTHPANE_TAB';
 export const REFRESH_QUERIES = 'REFRESH_QUERIES';
+export const USER_OFFLINE = 'USER_OFFLINE';


very good catch!

graceguo-supercat · 2018-10-12T05:05:50Z

superset/assets/src/SqlLab/components/SouthPane.jsx

@@ -39,6 +42,12 @@ class SouthPane extends React.PureComponent {
      latestQuery = props.editorQueries[props.editorQueries.length - 1];


if it is in offline state, you don't need to run these logic, you just render offline and exit early, right? so you can move offline check before this line.

graceguo-supercat · 2018-10-12T05:09:59Z

superset/assets/src/SqlLab/components/SouthPane.jsx

+    if (this.props.offline){
+      return (
+        <Label className="m-r-3" bsStyle={STATE_BSSTYLE_MAP['offline']}>
+          offline


instead of create a hard-coded text here, you should create new entry in STATUS_OPTIONS in constants.js

timifasubaa · 2018-10-18T04:39:04Z

Ready for review

graceguo-supercat · 2018-10-18T18:19:19Z

superset/assets/spec/javascripts/sqllab/TabbedSqlEditors_spec.jsx

@@ -166,4 +166,9 @@ describe('TabbedSqlEditors', () => {
    const lastTab = wrapper.find(Tab).last();
    expect(lastTab.props().eventKey).toContain('add_tab');
  });
+  it('should disable new tab when offline', () => {
+    wrapper = getWrapper();
+    wrapper.setProps({ offline: true });


before .setProps, can you add an expect that disabled props toBe(false)?

timifasubaa · 2018-10-20T00:39:16Z

Ready for stamp!

graceguo-supercat

LGTM

* add timeout and refresh for failed backend * show offline state instead of refreshing * add southpane tests

mistercrunch · 2019-01-30T06:37:33Z

FYI #6782

mistercrunch · 2019-01-31T17:47:31Z

@timifasubaa I'm not sure why, but in some cases when queries that return errors are executed, I see offline flash on the UI

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch 4 times, most recently from 120826e to ee89873 Compare October 1, 2018 18:47

graceguo-supercat reviewed Oct 1, 2018

View reviewed changes

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch from ee89873 to b3ef68f Compare October 3, 2018 20:10

timifasubaa changed the title ~~[Sqllab] add timeout and refresh for failed backend polling~~ [Sqllab] Add offline state to sqllab Oct 3, 2018

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch 7 times, most recently from da86a1f to f079f04 Compare October 9, 2018 07:34

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch 2 times, most recently from 70774eb to 34b1453 Compare October 11, 2018 08:49

graceguo-supercat reviewed Oct 11, 2018

View reviewed changes

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch 5 times, most recently from ced2f09 to 78f66fe Compare October 12, 2018 00:39

kristw reviewed Oct 12, 2018

View reviewed changes

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch from 78f66fe to b459143 Compare October 12, 2018 01:06

graceguo-supercat reviewed Oct 12, 2018

View reviewed changes

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch from b459143 to 734bcdb Compare October 12, 2018 17:46

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch 5 times, most recently from b6fd18e to 46009c3 Compare October 18, 2018 04:28

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch from 46009c3 to 7221cc5 Compare October 18, 2018 04:57

graceguo-supercat reviewed Oct 18, 2018

View reviewed changes

timifasubaa added 2 commits October 19, 2018 14:57

add timeout and refresh for failed backend

246b8d7

show offline state instead of refreshing

94f572f

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch 2 times, most recently from 2c1be7e to 81c4be4 Compare October 20, 2018 00:38

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch from 81c4be4 to 3879751 Compare October 20, 2018 01:26

add southpane tests

573ebac

timifasubaa force-pushed the sqllab_timeout_when_disconnected_from_server branch from 3879751 to 573ebac Compare October 22, 2018 23:47

graceguo-supercat approved these changes Oct 22, 2018

View reviewed changes

timifasubaa closed this Oct 23, 2018

timifasubaa reopened this Oct 23, 2018

timifasubaa closed this Oct 23, 2018

timifasubaa reopened this Oct 23, 2018

timifasubaa merged commit fc3b68e into apache:master Oct 23, 2018

bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018

[Sqllab] Add offline state to sqllab (apache#6013)

f0cac43

* add timeout and refresh for failed backend * show offline state instead of refreshing * add southpane tests

mistercrunch mentioned this pull request Jan 30, 2019

[sql lab] fix stuck offline #6782

Merged

graceguo-supercat mentioned this pull request Apr 29, 2019

[SQL Lab] Increase timeout threshold for offline check #7411

Merged

6 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sqllab] Add offline state to sqllab #6013

[Sqllab] Add offline state to sqllab #6013

timifasubaa commented Oct 1, 2018 •

edited

Loading

codecov-io commented Oct 1, 2018 •

edited

Loading

graceguo-supercat Oct 1, 2018

timifasubaa Oct 1, 2018

graceguo-supercat Oct 1, 2018

timifasubaa commented Oct 11, 2018

graceguo-supercat Oct 4, 2018 •

edited

Loading

timifasubaa Oct 12, 2018

graceguo-supercat Oct 9, 2018

timifasubaa Oct 12, 2018

graceguo-supercat Oct 11, 2018

timifasubaa Oct 12, 2018

graceguo-supercat Oct 11, 2018

timifasubaa Oct 12, 2018

timifasubaa commented Oct 12, 2018

kristw Oct 12, 2018

graceguo-supercat Oct 12, 2018

kristw Oct 12, 2018

graceguo-supercat Oct 12, 2018

graceguo-supercat Oct 12, 2018

graceguo-supercat Oct 12, 2018

timifasubaa commented Oct 18, 2018

graceguo-supercat Oct 18, 2018

timifasubaa Oct 20, 2018

timifasubaa commented Oct 20, 2018

graceguo-supercat left a comment

mistercrunch commented Jan 30, 2019

mistercrunch commented Jan 31, 2019

		@@ -39,6 +42,12 @@ class SouthPane extends React.PureComponent {
		latestQuery = props.editorQueries[props.editorQueries.length - 1];

[Sqllab] Add offline state to sqllab #6013

[Sqllab] Add offline state to sqllab #6013

Conversation

timifasubaa commented Oct 1, 2018 • edited Loading

codecov-io commented Oct 1, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Oct 11, 2018

graceguo-supercat Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Oct 20, 2018

graceguo-supercat left a comment

Choose a reason for hiding this comment

mistercrunch commented Jan 30, 2019

mistercrunch commented Jan 31, 2019

timifasubaa commented Oct 1, 2018 •

edited

Loading

codecov-io commented Oct 1, 2018 •

edited

Loading

graceguo-supercat Oct 4, 2018 •

edited

Loading