You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ma...@apache.org on 2024/12/05 23:27:02 UTC
(superset) branch sqla-close created (now 88b76df1cf)
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a change to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
at 88b76df1cf touchups
This branch includes the following new commits:
new e28727744e feat: temporarily disconnect metadata db during long analytics queries
new 1978759e1b impprovments
new f9ca78252f working
new c3cea52026 disabling NullPool used for testing
new a0113468e8 rm package-lock
new ecca931f55 docuement ff and set to false
new 88b76df1cf touchups
The 7 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 06/07: docuement ff and set to false
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ecca931f55c80566f544d4965dbcb7f9946db63c
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Thu Dec 5 11:33:36 2024 -0800
docuement ff and set to false
---
superset/config.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/superset/config.py b/superset/config.py
index 8d2ec7cc83..9ed61ad3d1 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -566,7 +566,9 @@ DEFAULT_FEATURE_FLAGS: dict[str, bool] = {
# If on, you'll want to add "https://avatars.slack-edge.com" to the list of allowed
# domains in your TALISMAN_CONFIG
"SLACK_ENABLE_AVATARS": False,
- "DISABLE_METADATA_DB_DURING_ANALYTICS": True,
+ # This feature flag works only in combination with NullPool, and disconnects the metadata db
+ # connection temporarily during the execution of analytics queries, avoiding bottlenecks
+ "DISABLE_METADATA_DB_DURING_ANALYTICS": False,
}
# ------------------------------
(superset) 01/07: feat: temporarily disconnect metadata db during long analytics queries
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e28727744e29b5b6457b450650b4223ff934ff95
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Wed Dec 4 17:36:34 2024 -0800
feat: temporarily disconnect metadata db during long analytics queries
---
superset/config.py | 1 +
superset/models/core.py | 72 ++++++++++++++++++++++++++++++++++---------------
superset/sql_lab.py | 5 ++--
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/superset/config.py b/superset/config.py
index 8a490a982d..a4dbe5a877 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -563,6 +563,7 @@ DEFAULT_FEATURE_FLAGS: dict[str, bool] = {
# If on, you'll want to add "https://avatars.slack-edge.com" to the list of allowed
# domains in your TALISMAN_CONFIG
"SLACK_ENABLE_AVATARS": False,
+ "DISABLE_METADATA_DB_DURING_ANALYTICS": True,
}
# ------------------------------
diff --git a/superset/models/core.py b/superset/models/core.py
index 9b06932afc..94e915f0d7 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -95,6 +95,35 @@ if TYPE_CHECKING:
DB_CONNECTION_MUTATOR = config["DB_CONNECTION_MUTATOR"]
+@contextmanager
+def temporarily_disconnect_metadata_db(): # type: ignore
+ """
+ Temporary disconnects the metadata database session during analytics query.
+ The goal here is to lower the number of concurrent connections to the metadata database,
+ given that Superset has no control over the duration of the analytics query.
+
+ If using a connection pool, the connn is release during the period. If not, the conn is closed
+ and has to be re-established.
+
+ Only has an effect if feature flag DISABLE_METADATA_DB_DURING_ANALYTICS is on
+ """
+ do_it = is_feature_enabled("DISABLE_METADATA_DB_DURING_ANALYTICS")
+ print("YO: temporarily_disconnect_metadata_db")
+ try:
+ if do_it:
+ print("YO: Disconnecting")
+ db.session.close()
+ yield None
+ finally:
+ if do_it:
+ print("YO: Reconnecting to metadata database")
+ conn = db.engine.connect()
+ print(f"YO: conn: {conn.closed}")
+ print(conn)
+ conn.open()
+ db.session = db._make_scoped_session()
+
+
class KeyValue(Model): # pylint: disable=too-few-public-methods
"""Used for any type of key-value store"""
@@ -691,27 +720,28 @@ class Database(Model, AuditMixinNullable, ImportExportMixin): # pylint: disable
with self.get_raw_connection(catalog=catalog, schema=schema) as conn:
cursor = conn.cursor()
df = None
- for i, sql_ in enumerate(sqls):
- sql_ = self.mutate_sql_based_on_config(sql_, is_split=True)
- _log_query(sql_)
- with event_logger.log_context(
- action="execute_sql",
- database=self,
- object_ref=__name__,
- ):
- self.db_engine_spec.execute(cursor, sql_, self)
- if i < len(sqls) - 1:
- # If it's not the last, we don't keep the results
- cursor.fetchall()
- else:
- # Last query, fetch and process the results
- data = self.db_engine_spec.fetch_data(cursor)
- result_set = SupersetResultSet(
- data, cursor.description, self.db_engine_spec
- )
- df = result_set.to_pandas_df()
- if mutator:
- df = mutator(df)
+ with temporarily_disconnect_metadata_db():
+ for i, sql_ in enumerate(sqls):
+ sql_ = self.mutate_sql_based_on_config(sql_, is_split=True)
+ _log_query(sql_)
+ with event_logger.log_context(
+ action="execute_sql",
+ database=self,
+ object_ref=__name__,
+ ):
+ self.db_engine_spec.execute(cursor, sql_, self)
+ if i < len(sqls) - 1:
+ # If it's not the last, we don't keep the results
+ cursor.fetchall()
+ else:
+ # Last query, fetch and process the results
+ data = self.db_engine_spec.fetch_data(cursor)
+ result_set = SupersetResultSet(
+ data, cursor.description, self.db_engine_spec
+ )
+ df = result_set.to_pandas_df()
+ if mutator:
+ df = mutator(df)
return self.post_process_df(df)
diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index 88e1bc1aad..537fe9ca1d 100644
--- a/superset/sql_lab.py
+++ b/superset/sql_lab.py
@@ -50,7 +50,7 @@ from superset.exceptions import (
SupersetParseError,
)
from superset.extensions import celery_app, event_logger
-from superset.models.core import Database
+from superset.models.core import Database, temporarily_disconnect_metadata_db
from superset.models.sql_lab import Query
from superset.result_set import SupersetResultSet
from superset.sql.parse import SQLStatement, Table
@@ -304,7 +304,8 @@ def execute_sql_statement( # pylint: disable=too-many-statements, too-many-loca
object_ref=__name__,
):
with stats_timing("sqllab.query.time_executing_query", stats_logger):
- db_engine_spec.execute_with_cursor(cursor, sql, query)
+ with temporarily_disconnect_metadata_db():
+ db_engine_spec.execute_with_cursor(cursor, sql, query)
with stats_timing("sqllab.query.time_fetching_results", stats_logger):
logger.debug(
(superset) 05/07: rm package-lock
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit a0113468e89fe4e8a10e58cc0462a3438b09c248
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Thu Dec 5 11:23:01 2024 -0800
rm package-lock
---
superset-frontend/package-lock.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
index f4bef44036..99e08f8d59 100644
--- a/superset-frontend/package-lock.json
+++ b/superset-frontend/package-lock.json
@@ -69182,7 +69182,7 @@
"@types/jest": "^29.5.12",
"@types/lodash": "^4.17.7",
"handlebars": "^4.7.8",
- "handlebars-group-by": "^1.0.1",
+ "handlebars-group-by": "*",
"jest": "^29.7.0",
"just-handlebars-helpers": "^1.0.19"
},
(superset) 02/07: impprovments
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1978759e1bf0bcd001f699af3b80fcd27adf3ebb
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Thu Dec 5 09:08:14 2024 -0800
impprovments
---
superset-frontend/package-lock.json | 2 +-
superset/config.py | 4 +++-
superset/models/core.py | 35 +++++++++++++++++++++++++++--------
superset/sql_lab.py | 5 ++---
superset/sqllab/sql_json_executer.py | 8 +++++++-
5 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
index 99e08f8d59..f4bef44036 100644
--- a/superset-frontend/package-lock.json
+++ b/superset-frontend/package-lock.json
@@ -69182,7 +69182,7 @@
"@types/jest": "^29.5.12",
"@types/lodash": "^4.17.7",
"handlebars": "^4.7.8",
- "handlebars-group-by": "*",
+ "handlebars-group-by": "^1.0.1",
"jest": "^29.7.0",
"just-handlebars-helpers": "^1.0.19"
},
diff --git a/superset/config.py b/superset/config.py
index a4dbe5a877..f9d10aca18 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -204,7 +204,9 @@ SQLALCHEMY_DATABASE_URI = (
# `SQLALCHEMY_ENGINE_OPTIONS = {"isolation_level": "READ COMMITTED"}`
# Also note that we recommend READ COMMITTED for regular operation.
# Find out more here https://flask-sqlalchemy.palletsprojects.com/en/3.1.x/config/
-SQLALCHEMY_ENGINE_OPTIONS = {}
+from sqlalchemy.pool import NullPool
+
+SQLALCHEMY_ENGINE_OPTIONS = {"poolclass": NullPool}
# In order to hook up a custom password store for all SQLALCHEMY connections
# implement a function that takes a single argument of type 'sqla.engine.url',
diff --git a/superset/models/core.py b/superset/models/core.py
index 94e915f0d7..4e60ba0cb8 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -95,6 +95,17 @@ if TYPE_CHECKING:
DB_CONNECTION_MUTATOR = config["DB_CONNECTION_MUTATOR"]
+def print_connection_status():
+ logger.info("Pinging database")
+ logger.info(f"STATUS: {db.session.connection().closed}")
+
+ try:
+ db.session.execute("SELECT 1")
+ logger.info("success")
+ except Exception:
+ logger.error("connection failed as expected")
+
+
@contextmanager
def temporarily_disconnect_metadata_db(): # type: ignore
"""
@@ -108,20 +119,28 @@ def temporarily_disconnect_metadata_db(): # type: ignore
Only has an effect if feature flag DISABLE_METADATA_DB_DURING_ANALYTICS is on
"""
do_it = is_feature_enabled("DISABLE_METADATA_DB_DURING_ANALYTICS")
- print("YO: temporarily_disconnect_metadata_db")
+ connection = db.session.connection()
try:
if do_it:
- print("YO: Disconnecting")
+ logger.info("Disconnecting metadata database temporarily")
db.session.close()
+ print_connection_status()
+ connection.close()
+ logger.info(f"STATUS: {connection.closed}")
yield None
finally:
if do_it:
- print("YO: Reconnecting to metadata database")
- conn = db.engine.connect()
- print(f"YO: conn: {conn.closed}")
- print(conn)
- conn.open()
- db.session = db._make_scoped_session()
+ logger.info("Reconnecting to metadata database")
+ logger.info("BEFORE")
+ print_connection_status()
+
+ # NOTE: Interface changes in flask-sqlalchemy 3.0
+
+ db.session.connection()
+ db.session = db.create_scoped_session()
+
+ logger.info("AFTER")
+ print_connection_status()
class KeyValue(Model): # pylint: disable=too-few-public-methods
diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index 537fe9ca1d..88e1bc1aad 100644
--- a/superset/sql_lab.py
+++ b/superset/sql_lab.py
@@ -50,7 +50,7 @@ from superset.exceptions import (
SupersetParseError,
)
from superset.extensions import celery_app, event_logger
-from superset.models.core import Database, temporarily_disconnect_metadata_db
+from superset.models.core import Database
from superset.models.sql_lab import Query
from superset.result_set import SupersetResultSet
from superset.sql.parse import SQLStatement, Table
@@ -304,8 +304,7 @@ def execute_sql_statement( # pylint: disable=too-many-statements, too-many-loca
object_ref=__name__,
):
with stats_timing("sqllab.query.time_executing_query", stats_logger):
- with temporarily_disconnect_metadata_db():
- db_engine_spec.execute_with_cursor(cursor, sql, query)
+ db_engine_spec.execute_with_cursor(cursor, sql, query)
with stats_timing("sqllab.query.time_fetching_results", stats_logger):
logger.debug(
diff --git a/superset/sqllab/sql_json_executer.py b/superset/sqllab/sql_json_executer.py
index 27483fb31c..9ad70ca6ab 100644
--- a/superset/sqllab/sql_json_executer.py
+++ b/superset/sqllab/sql_json_executer.py
@@ -31,6 +31,7 @@ from superset.exceptions import (
SupersetGenericDBErrorException,
SupersetTimeoutException,
)
+from superset.models.core import temporarily_disconnect_metadata_db
from superset.sqllab.command_status import SqlJsonExecutionStatus
from superset.utils import core as utils
from superset.utils.core import get_username
@@ -100,7 +101,9 @@ class SynchronousSqlJsonExecutor(SqlJsonExecutorBase):
except SupersetTimeoutException:
raise
except Exception as ex:
+ raise ex
logger.exception("Query %i failed unexpectedly", query_id)
+ logger.error(ex)
raise SupersetGenericDBErrorException(
utils.error_msg_from_exception(ex)
) from ex
@@ -127,7 +130,10 @@ class SynchronousSqlJsonExecutor(SqlJsonExecutorBase):
seconds=self._timeout_duration_in_seconds,
error_message=self._get_timeout_error_msg(),
):
- return self._get_sql_results(execution_context, rendered_query, log_params)
+ with temporarily_disconnect_metadata_db():
+ return self._get_sql_results(
+ execution_context, rendered_query, log_params
+ )
def _get_sql_results(
self,
(superset) 03/07: working
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit f9ca78252f11ad3f21e1f60886bbcf5962c31690
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Thu Dec 5 10:45:52 2024 -0800
working
---
superset/models/core.py | 55 +++++++++++++++---------------------
superset/sqllab/sql_json_executer.py | 5 ++--
2 files changed, 25 insertions(+), 35 deletions(-)
diff --git a/superset/models/core.py b/superset/models/core.py
index 4e60ba0cb8..7d5b688a18 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -95,53 +95,44 @@ if TYPE_CHECKING:
DB_CONNECTION_MUTATOR = config["DB_CONNECTION_MUTATOR"]
-def print_connection_status():
- logger.info("Pinging database")
- logger.info(f"STATUS: {db.session.connection().closed}")
-
- try:
- db.session.execute("SELECT 1")
- logger.info("success")
- except Exception:
- logger.error("connection failed as expected")
-
-
@contextmanager
-def temporarily_disconnect_metadata_db(): # type: ignore
+def temporarily_disconnect_db(): # type: ignore
"""
- Temporary disconnects the metadata database session during analytics query.
+ Temporary disconnects the metadata database session.
+
+ This is meant to be used during long, blocking operations, so that we can release
+ the database connection for the duration of, for example, a potentially long running
+ query against an analytics database.
+
The goal here is to lower the number of concurrent connections to the metadata database,
given that Superset has no control over the duration of the analytics query.
- If using a connection pool, the connn is release during the period. If not, the conn is closed
- and has to be re-established.
-
- Only has an effect if feature flag DISABLE_METADATA_DB_DURING_ANALYTICS is on
+ NOTE: only has an effect if feature flag DISABLE_METADATA_DB_DURING_ANALYTICS
+ and using NullPool
"""
- do_it = is_feature_enabled("DISABLE_METADATA_DB_DURING_ANALYTICS")
- connection = db.session.connection()
+ pool_type = db.engine.pool.__class__.__name__
+ # Currently only tested/available when used with NullPool
+ do_it = (
+ is_feature_enabled("DISABLE_METADATA_DB_DURING_ANALYTICS")
+ and pool_type == "NullPool"
+ )
+ conn = db.session.connection()
try:
if do_it:
logger.info("Disconnecting metadata database temporarily")
+ # Closing the session
db.session.close()
- print_connection_status()
- connection.close()
- logger.info(f"STATUS: {connection.closed}")
+ # Closing the connection
+ conn.close()
yield None
finally:
if do_it:
logger.info("Reconnecting to metadata database")
- logger.info("BEFORE")
- print_connection_status()
-
- # NOTE: Interface changes in flask-sqlalchemy 3.0
-
- db.session.connection()
+ conn = db.session.connection()
+ # Creating a new scoped session
+ # NOTE: Interface changes in flask-sqlalchemy ~3.0
db.session = db.create_scoped_session()
- logger.info("AFTER")
- print_connection_status()
-
class KeyValue(Model): # pylint: disable=too-few-public-methods
"""Used for any type of key-value store"""
@@ -739,7 +730,7 @@ class Database(Model, AuditMixinNullable, ImportExportMixin): # pylint: disable
with self.get_raw_connection(catalog=catalog, schema=schema) as conn:
cursor = conn.cursor()
df = None
- with temporarily_disconnect_metadata_db():
+ with temporarily_disconnect_db():
for i, sql_ in enumerate(sqls):
sql_ = self.mutate_sql_based_on_config(sql_, is_split=True)
_log_query(sql_)
diff --git a/superset/sqllab/sql_json_executer.py b/superset/sqllab/sql_json_executer.py
index 9ad70ca6ab..0c21410fe0 100644
--- a/superset/sqllab/sql_json_executer.py
+++ b/superset/sqllab/sql_json_executer.py
@@ -31,7 +31,7 @@ from superset.exceptions import (
SupersetGenericDBErrorException,
SupersetTimeoutException,
)
-from superset.models.core import temporarily_disconnect_metadata_db
+from superset.models.core import temporarily_disconnect_db
from superset.sqllab.command_status import SqlJsonExecutionStatus
from superset.utils import core as utils
from superset.utils.core import get_username
@@ -101,7 +101,6 @@ class SynchronousSqlJsonExecutor(SqlJsonExecutorBase):
except SupersetTimeoutException:
raise
except Exception as ex:
- raise ex
logger.exception("Query %i failed unexpectedly", query_id)
logger.error(ex)
raise SupersetGenericDBErrorException(
@@ -130,7 +129,7 @@ class SynchronousSqlJsonExecutor(SqlJsonExecutorBase):
seconds=self._timeout_duration_in_seconds,
error_message=self._get_timeout_error_msg(),
):
- with temporarily_disconnect_metadata_db():
+ with temporarily_disconnect_db():
return self._get_sql_results(
execution_context, rendered_query, log_params
)
(superset) 07/07: touchups
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 88b76df1cf3d415fd7879169b8bf221a7556d454
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Thu Dec 5 15:26:22 2024 -0800
touchups
---
superset/config.py | 1 +
superset/sqllab/sql_json_executer.py | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/superset/config.py b/superset/config.py
index 9ed61ad3d1..c5d3199873 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -207,6 +207,7 @@ SQLALCHEMY_DATABASE_URI = (
# for example, if you don't want to use a connection pool, uncomment those next 2 lines
# from sqlalchemy.pool import NullPool
# SQLALCHEMY_ENGINE_OPTIONS = {"poolclass": NullPool}
+SQLALCHEMY_ENGINE_OPTIONS = {}
# In order to hook up a custom password store for all SQLALCHEMY connections
diff --git a/superset/sqllab/sql_json_executer.py b/superset/sqllab/sql_json_executer.py
index 0c21410fe0..02396fb75d 100644
--- a/superset/sqllab/sql_json_executer.py
+++ b/superset/sqllab/sql_json_executer.py
@@ -102,7 +102,6 @@ class SynchronousSqlJsonExecutor(SqlJsonExecutorBase):
raise
except Exception as ex:
logger.exception("Query %i failed unexpectedly", query_id)
- logger.error(ex)
raise SupersetGenericDBErrorException(
utils.error_msg_from_exception(ex)
) from ex
(superset) 04/07: disabling NullPool used for testing
Posted by ma...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch sqla-close
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c3cea52026ed038c969f341a6c6d57a681a194bf
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Thu Dec 5 11:07:39 2024 -0800
disabling NullPool used for testing
---
superset/config.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/superset/config.py b/superset/config.py
index f9d10aca18..8d2ec7cc83 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -204,9 +204,10 @@ SQLALCHEMY_DATABASE_URI = (
# `SQLALCHEMY_ENGINE_OPTIONS = {"isolation_level": "READ COMMITTED"}`
# Also note that we recommend READ COMMITTED for regular operation.
# Find out more here https://flask-sqlalchemy.palletsprojects.com/en/3.1.x/config/
-from sqlalchemy.pool import NullPool
+# for example, if you don't want to use a connection pool, uncomment those next 2 lines
+# from sqlalchemy.pool import NullPool
+# SQLALCHEMY_ENGINE_OPTIONS = {"poolclass": NullPool}
-SQLALCHEMY_ENGINE_OPTIONS = {"poolclass": NullPool}
# In order to hook up a custom password store for all SQLALCHEMY connections
# implement a function that takes a single argument of type 'sqla.engine.url',