[GitHub] [superset] villebro commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)

[GitBox <gi...@apache.org>]

villebro commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1062194520


##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -235,7 +235,7 @@ export function querySuccess(query, results) {
       !query.isDataPreview &&
       isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
         ? SupersetClient.put({
-            endpoint: encodeURI(`/tabstateview/${sqlEditorId}`),
+            endpoint: encodeURI(`/tabstateview/${results?.query?.sqlEditorId}`),

Review Comment:
   If you look at line 232, a recent PR of mine recently changed this logic: #22498 So this line needs to be changed back:
   ```suggestion
               endpoint: encodeURI(`/tabstateview/${sqlEditorId}`),
   ```



##########
superset/config.py:
##########
@@ -1107,6 +1107,8 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC(  # pylint: disable=invalid-name
 
 # Interval between consecutive polls when using Hive Engine
 HIVE_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
+# Interval between consecutive polls when using Impala Engine
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())

Review Comment:
   To make sure we don't clutter `config.py` with too many disparate config keys, maybe we should remove these `*_POLL_INTERVAL` keys and refactor this to something like
   ```python
   DB_POLL_INTERVAL_SECONDS: Dict[str, int] = {}
   ```
   This could be used to specify these per engine name in your `superset_config.py` (here I'd be overriding polling to 1 seconds for Hive):
   ```python
   DB_POLL_INTERVAL_SECONDS = {
       "hive": int(timedelta(seconds=1).total_seconds()),
   }
   ```
   I know it's a breaking change, so we can probably fall back to `HIVE_POLL_INTERVAL` in the hive spec for now. So maybe change the following line https://github.com/apache/superset/blob/01671b9d1b3a15c264bcfb9eced1776c70e293b5/superset/db_engine_specs/hive.py#L378 to something like this (see how I've moved the default to the engine spec, rather than having it in `config.py`):
   
   ```python
   if sleep_interval := current_app.config.get("HIVE_POLL_INTERVAL"):
       logger.warning("HIVE_POLL_INTERVAL is deprecated and will be removed in 3.0. Please use DB_POLL_INTERVAL instead")
   else:
       sleep_interval = current_app.config["DB_POLL_INTERVAL_SECONDS"].get(cls.engine, 5)
   
   time.sleep(sleep_interval)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org