justinpark commented on PR #43371: URL: https://github.com/apache/superset/pull/43371#issuecomment-5417045809
> 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. Very nice suggestion! I've updated the commit accordingly. -- 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]
