kaxil commented on code in PR #75327:
URL: https://github.com/apache/airflow/pull/75327#discussion_r4341475997
##########
providers/common/sql/src/airflow/providers/common/sql/hooks/sql.py:
##########
@@ -378,7 +380,10 @@ def inspector(self) -> Inspector:
"SQLAlchemy is required for database inspection. "
"Install it with: pip install
'apache-airflow-providers-common-sql[sqlalchemy]'"
)
- return inspect(self.get_sqlalchemy_engine())
+ # The inspector is cached for the hook's lifetime, so a pooled engine
would keep the
+ # connection it reflected with open until the process exits. NullPool
closes it after
+ # each inspection instead, like run() and get_records() do.
+ return inspect(self.get_sqlalchemy_engine({"poolclass": NullPool}))
Review Comment:
Good catch, and you're right that the cache doesn't save the connect. The
Inspector opens the connection before the cached dialect method runs. I get the
same numbers: 5 tables with `get_columns`/`get_foreign_keys`/`get_indexes`, 17
connects on NullPool vs 1 on main (SQLAlchemy 2.0.54). For comparison, the
per-access engine from before #62594 did 16 for the same loop, so this goes
back to the connection count `inspector` had before common.sql 1.33.0, while
keeping one engine per hook.
I'd still rather the default not leak. `DbApiHook` has no task-end hook to
call `dispose()` from, so keeping the pool means every caller that doesn't
dispose (`insert_rows` resolving target fields, the Db2 upsert) holds a
connection until exit. For a per-table loop like `LLMSchemaCompareOperator`, I
think the better fix is reflecting over one connection (`inspect(conn)` inside
`engine.connect()`), which was 1 connect / 1 close in the same probe. I'll do
that as a follow-up in common.ai.
Updated the Cost paragraph in the description with the numbers.
##########
providers/common/sql/src/airflow/providers/common/sql/hooks/sql.py:
##########
@@ -378,7 +380,10 @@ def inspector(self) -> Inspector:
"SQLAlchemy is required for database inspection. "
"Install it with: pip install
'apache-airflow-providers-common-sql[sqlalchemy]'"
)
- return inspect(self.get_sqlalchemy_engine())
+ # The inspector is cached for the hook's lifetime, so a pooled engine
would keep the
+ # connection it reflected with open until the process exits. NullPool
closes it after
+ # each inspection instead, like run() and get_records() do.
+ return inspect(self.get_sqlalchemy_engine({"poolclass": NullPool}))
Review Comment:
I think we're fine leaving it. `engine_kwargs=None` has been on the base
`get_sqlalchemy_engine` since it was added, so a parameterless override already
breaks any caller that passes kwargs. An override that assigns its own
`poolclass` into `engine_kwargs` wins over ours, which seems right. The one
case that would break is an override passing `poolclass=` directly alongside
`**engine_kwargs` (duplicate keyword). I couldn't find one in tree.
--
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]
