pankajkoti commented on code in PR #75327:
URL: https://github.com/apache/airflow/pull/75327#discussion_r4341096774
##########
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:
Thanks for tracking this down, the leak is a good catch. One thing I noticed
while trying this locally on SQLAlchemy 2.1.6 and wanted to check with you.
With `NullPool`, each reflection call seems to open its own connection, even
when the Inspector already has the result cached. Reflecting 5 tables
(`get_columns`, `get_foreign_keys`, `get_indexes`) took 17 connects for me,
against 1 on the default pool. Since `LLMSchemaCompareOperator` reflects per
table, I wondered how that plays out on something like Snowflake with a large
schema. Do you think that is acceptable for the typical case? I was also
wondering whether keeping the pool and disposing the engine at the end of the
task could be a good middle ground. Happy to be wrong here if I'm missing
something.
##########
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:
Last one, and it is minor. With `sqlite:///:memory:` the default pool
reflects a table created through that engine, but under `NullPool` each
connection starts with an empty database. I don't think `SqliteHook` is
affected, since `get_conn` uses its own `sqlite3.connect`. Do you know of any
case where an in-memory database is used with `hook.inspector`?
##########
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.
Review Comment:
Nice explanation of the why. A small wording thought: `run()` and
`get_records()` close a raw DBAPI connection through
`closing(self.get_conn())`, so they don't use a pool at all. Did you mean the
outcome is the same (closed after use)? Something like "so no connection
outlives an inspection" might avoid implying the same mechanism. Totally your
call.
##########
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:
Another small thing I was curious about. The inspector now passes
`{"poolclass": NullPool}` to `get_sqlalchemy_engine`, where it passed nothing
before. All the in-tree overrides forward `engine_kwargs`, so I think we're
fine there. Do you think external hooks with a parameterless override, or one
that sets its own `poolclass`, are worth worrying about? I can see an argument
for leaving it as is.
--
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]
