Deprecate database attribute allow_run_sync (apache#4961)

* Deprecate database attribute allow_run_sync There's really 2 modes of operations in SQL Lab, sync or async though currently there are 2 boolean flags: allow_run_sync and allow_run_async, leading to 4 potential combinations, only 2 of which actually makes sense. The original vision is that we'd expose the choice to users and they would decide which `Run` or `Run Async` button to hit. Later on we decided to have a single button and for each database to be either sync or async. This PR cleans up allow_run_sync by removing references to it. * Fix build * Add db migration * using batch_op (cherry picked from commit 2931baa) (cherry picked from commit d07786e)
lyft · Jan 11, 2019 · b180e76 · b180e76
1 parent 92601bc
commit b180e76
Show file tree

Hide file tree

Showing 8 changed files with 82 additions and 66 deletions.
diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js
@@ -292,7 +292,6 @@ export const databases = {
       allow_ctas: true,
       allow_dml: true,
       allow_run_async: false,
-      allow_run_sync: true,
       database_name: 'main',
       expose_in_sqllab: true,
       force_ctas_schema: '',
@@ -302,7 +301,6 @@ export const databases = {
       allow_ctas: true,
       allow_dml: false,
       allow_run_async: true,
-      allow_run_sync: true,
       database_name: 'Presto - Gold',
       expose_in_sqllab: true,
       force_ctas_schema: 'tmp',

diff --git a/superset/assets/src/SqlLab/components/RunQueryActionButton.jsx b/superset/assets/src/SqlLab/components/RunQueryActionButton.jsx
@@ -29,7 +29,28 @@ export default function RunQueryActionButton(props) {
     disabled: !(props.dbId),
   };
 
-  const syncBtn = (
+  if (shouldShowStopBtn) {
+    return (
+      <Button
+        {...commonBtnProps}
+        onClick={props.stopQuery}
+      >
+        <i className="fa fa-stop" /> {t('Stop')}
+      </Button>
+    );
+  } else if (props.allowAsync) {
+    return (
+      <Button
+        {...commonBtnProps}
+        onClick={() => props.runQuery(true)}
+        key="run-async-btn"
+        tooltip={t('Run query asynchronously')}
+        disabled={!props.sql.trim()}
+      >
+        <i className="fa fa-table" /> {runBtnText}
+      </Button>);
+  }
+  return (
     <Button
       {...commonBtnProps}
       onClick={() => props.runQuery(false)}
@@ -40,37 +61,6 @@ export default function RunQueryActionButton(props) {
       <i className="fa fa-refresh" /> {runBtnText}
     </Button>
   );
-
-  const asyncBtn = (
-    <Button
-      {...commonBtnProps}
-      onClick={() => props.runQuery(true)}
-      key="run-async-btn"
-      tooltip={t('Run query asynchronously')}
-      disabled={!props.sql.trim()}
-    >
-      <i className="fa fa-table" /> {runBtnText}
-    </Button>
-  );
-
-  const stopBtn = (
-    <Button
-      {...commonBtnProps}
-      onClick={props.stopQuery}
-    >
-      <i className="fa fa-stop" /> {t('Stop')}
-    </Button>
-  );
-
-  let button;
-  if (shouldShowStopBtn) {
-    button = stopBtn;
-  } else if (props.allowAsync) {
-    button = asyncBtn;
-  } else {
-    button = syncBtn;
-  }
-  return button;
 }
 
 RunQueryActionButton.propTypes = propTypes;

diff --git a/superset/assets/src/SqlLab/components/SqlEditor.jsx b/superset/assets/src/SqlLab/components/SqlEditor.jsx
@@ -137,7 +137,9 @@ class SqlEditor extends React.PureComponent {
     this.props.actions.queryEditorSetQueryLimit(this.props.queryEditor, queryLimit);
   }
   runQuery() {
-    this.startQuery(!(this.props.database || {}).allow_run_sync);
+    if (this.props.database) {
+      this.startQuery(this.props.database.allow_run_async);
+    }
   }
   startQuery(runAsync = false, ctas = false) {
     const qe = this.props.queryEditor;

diff --git a/superset/migrations/versions/46f444d8b9b7_remove_coordinator_from_druid_cluster_.py b/superset/migrations/versions/46f444d8b9b7_remove_coordinator_from_druid_cluster_.py
@@ -5,27 +5,28 @@
 Create Date: 2018-11-26 00:01:04.781119
 
 """
+from alembic import op
+import sqlalchemy as sa
 
 # revision identifiers, used by Alembic.
 revision = '46f444d8b9b7'
 down_revision = '4ce8df208545'
 
-from alembic import op
-import sqlalchemy as sa
-import logging
-
 
 def upgrade():
-    try:
-        op.drop_column('clusters', 'coordinator_host')
-        op.drop_column('clusters', 'coordinator_endpoint')
-        op.drop_column('clusters', 'coordinator_port')
-    except Exception as e:
-        # Sqlite does not support drop column
-        logging.warning(str(e))
+    with op.batch_alter_table('clusters') as batch_op:
+        batch_op.drop_column('coordinator_host')
+        batch_op.drop_column('coordinator_endpoint')
+        batch_op.drop_column('coordinator_port')
 
 
 def downgrade():
-    op.add_column('clusters', sa.Column('coordinator_host', sa.String(length=256), nullable=True))
+    op.add_column(
+        'clusters',
+        sa.Column('coordinator_host', sa.String(length=256), nullable=True),
+    )
     op.add_column('clusters', sa.Column('coordinator_port', sa.Integer(), nullable=True))
-    op.add_column('clusters', sa.Column('coordinator_endpoint', sa.String(length=256), nullable=True))
+    op.add_column(
+        'clusters',
+        sa.Column('coordinator_endpoint', sa.String(length=256), nullable=True),
+    )
diff --git a/superset/migrations/versions/a61b40f9f57f_remove_allow_run_sync.py b/superset/migrations/versions/a61b40f9f57f_remove_allow_run_sync.py
@@ -0,0 +1,29 @@
+"""remove allow_run_sync
+
+Revision ID: a61b40f9f57f
+Revises: 46f444d8b9b7
+Create Date: 2018-11-27 11:53:17.512627
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+# revision identifiers, used by Alembic.
+revision = 'a61b40f9f57f'
+down_revision = '46f444d8b9b7'
+
+
+def upgrade():
+    with op.batch_alter_table('dbs') as batch_op:
+        batch_op.drop_column('allow_run_sync')
+
+
+def downgrade():
+    op.add_column(
+        'dbs',
+        sa.Column(
+            'allow_run_sync',
+            sa.Integer(display_width=1),
+            autoincrement=False, nullable=True,
+        ),
+    )
diff --git a/superset/models/core.py b/superset/models/core.py
@@ -638,8 +638,7 @@ class Database(Model, AuditMixinNullable, ImportMixin):
     password = Column(EncryptedType(String(1024), config.get('SECRET_KEY')))
     cache_timeout = Column(Integer)
     select_as_create_table_as = Column(Boolean, default=False)
-    expose_in_sqllab = Column(Boolean, default=False)
-    allow_run_sync = Column(Boolean, default=True)
+    expose_in_sqllab = Column(Boolean, default=True)
     allow_run_async = Column(Boolean, default=False)
     allow_csv_upload = Column(Boolean, default=False)
     allow_ctas = Column(Boolean, default=False)
@@ -657,7 +656,7 @@ class Database(Model, AuditMixinNullable, ImportMixin):
     perm = Column(String(1000))
     impersonate_user = Column(Boolean, default=False)
     export_fields = ('database_name', 'sqlalchemy_uri', 'cache_timeout',
-                     'expose_in_sqllab', 'allow_run_sync', 'allow_run_async',
+                     'expose_in_sqllab', 'allow_run_async',
                      'allow_ctas', 'allow_csv_upload', 'extra')
     export_children = ['tables']
 

diff --git a/superset/utils/core.py b/superset/utils/core.py
@@ -838,7 +838,6 @@ def get_or_create_main_db():
         dbobj = models.Database(database_name='main')
     dbobj.set_sqlalchemy_uri(conf.get('SQLALCHEMY_DATABASE_URI'))
     dbobj.expose_in_sqllab = True
-    dbobj.allow_run_sync = True
     dbobj.allow_csv_upload = True
     db.session.add(dbobj)
     db.session.commit()

diff --git a/superset/views/core.py b/superset/views/core.py
@@ -142,15 +142,15 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin):  # noqa
     edit_title = _('Edit Database')
 
     list_columns = [
-        'database_name', 'backend', 'allow_run_sync', 'allow_run_async',
-        'allow_dml', 'allow_csv_upload', 'creator', 'modified']
+        'database_name', 'backend', 'allow_run_async',
+        'allow_dml', 'allow_csv_upload', 'expose_in_sqllab', 'creator', 'modified']
     order_columns = [
-        'database_name', 'allow_run_sync', 'allow_run_async', 'allow_dml',
-        'modified', 'allow_csv_upload',
+        'database_name', 'allow_run_async', 'allow_dml',
+        'modified', 'allow_csv_upload', 'expose_in_sqllab',
     ]
     add_columns = [
         'database_name', 'sqlalchemy_uri', 'cache_timeout', 'expose_in_sqllab',
-        'allow_run_sync', 'allow_run_async', 'allow_csv_upload',
+        'allow_run_async', 'allow_csv_upload',
         'allow_ctas', 'allow_dml', 'force_ctas_schema', 'impersonate_user',
         'allow_multi_schema_metadata_fetch', 'extra',
     ]
@@ -181,14 +181,13 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin):  # noqa
             'database-urls) '
             'for more information on how to structure your URI.', True),
         'expose_in_sqllab': _('Expose this DB in SQL Lab'),
-        'allow_run_sync': _(
-            'Allow users to run synchronous queries, this is the default '
-            'and should work well for queries that can be executed '
-            'within a web request scope (<~1 minute)'),
         'allow_run_async': _(
-            'Allow users to run queries, against an async backend. '
+            'Operate the database in asynchronous mode, meaning  '
+            'that the queries are executed on remote workers as opposed '
+            'to on the web server itself. '
             'This assumes that you have a Celery worker setup as well '
-            'as a results backend.'),
+            'as a results backend. Refer to the installation docs '
+            'for more information.'),
         'allow_ctas': _('Allow CREATE TABLE AS option in SQL Lab'),
         'allow_dml': _(
             'Allow users to run non-SELECT statements '
@@ -246,8 +245,7 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin):  # noqa
         'sqlalchemy_uri': _('SQLAlchemy URI'),
         'cache_timeout': _('Chart Cache Timeout'),
         'extra': _('Extra'),
-        'allow_run_sync': _('Allow Run Sync'),
-        'allow_run_async': _('Allow Run Async'),
+        'allow_run_async': _('Asynchronous Query Execution'),
         'impersonate_user': _('Impersonate the logged on user'),
         'allow_csv_upload': _('Allow Csv Upload'),
         'modified': _('Modified'),
@@ -317,7 +315,7 @@ class DatabaseAsync(DatabaseView):
     list_columns = [
         'id', 'database_name',
         'expose_in_sqllab', 'allow_ctas', 'force_ctas_schema',
-        'allow_run_async', 'allow_run_sync', 'allow_dml',
+        'allow_run_async', 'allow_dml',
         'allow_multi_schema_metadata_fetch', 'allow_csv_upload',
         'allows_subquery',
     ]