michael-s-molina commented on PR #43371:
URL: https://github.com/apache/superset/pull/43371#issuecomment-5415235135
Good fix for the connection-pool exhaustion. One suggestion on the
mechanism: `db.session.close()` detaches *every* object loaded in this
request's session (not just `self._query`), including `g.user` — wider blast
radius than needed, and easy to misread later.
Superset already has a narrower pattern for this in `execute_sql_statement`
(`superset/sql_lab.py`): release the connection via `commit()` with
`expire_on_commit` toggled off, instead of `close()`. That keeps objects
attached to the session (just un-expired) rather than fully detached:
```python
db.session.expire_on_commit = False
try:
_ = self._query.database
db.session.commit()
finally:
db.session.expire_on_commit = True
```
Could we pull this into a small shared utility (e.g.
`warm_and_release_connection(obj, "database")`) and have both `sql_lab.py` and
this command call it, instead of each having its own inline version?
Separately: all tests here patch `results_backend_use_msgpack` to `False`,
but `query.database.db_engine_spec` is only used in the `use_msgpack=True`
branch of `_deserialize_results_payload` — which is the production default.
Worth one end-to-end `run()` test with msgpack on.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
