Copilot commented on code in PR #64888:
URL: https://github.com/apache/airflow/pull/64888#discussion_r3051311176
##########
airflow-core/src/airflow/settings.py:
##########
@@ -510,8 +510,12 @@ def prepare_engine_args(disable_connection_pool=False,
pool_class=None):
elif disable_connection_pool or not conf.getboolean("database",
"SQL_ALCHEMY_POOL_ENABLED"):
engine_args["poolclass"] = NullPool
log.debug("settings.prepare_engine_args(): Using NullPool")
- elif not SQL_ALCHEMY_CONN.startswith("sqlite"):
- # Pool size engine args not supported by sqlite.
+ elif SQL_ALCHEMY_CONN in ("sqlite://", "sqlite:///:memory:"):
+ # In-memory SQLite uses SingletonThreadPool which doesn't support
pool_size/max_overflow.
+ log.debug("settings.prepare_engine_args(): Skipping pool settings for
in-memory SQLite")
+ else:
+ # Pool settings for all file-based databases including SQLite.
+ # SQLAlchemy 2.0+ uses QueuePool by default for file-based SQLite.
Review Comment:
The in-memory SQLite detection is too strict: it only matches exact strings
"sqlite://" and "sqlite:///:memory:". Connection URLs can include a driver
prefix (e.g. "sqlite+pysqlite:///:memory:"), additional query params (e.g.
"sqlite:///:memory:?cache=shared"), or other in-memory forms (e.g.
"sqlite:///file::memory:?cache=shared"). In those cases this branch won’t run
and Airflow will apply pool_size/max_overflow, which can raise at engine
creation with SingletonThreadPool. Consider parsing the URL (e.g. via
SQLAlchemy URL parsing) and checking the dialect is sqlite and the database
points to an in-memory target, rather than doing string equality.
##########
airflow-core/tests/unit/core/test_sqlalchemy_config.py:
##########
@@ -70,6 +70,43 @@ def test_configure_orm_with_default_values(
**expected_kwargs,
)
+ @patch("airflow.settings.setup_event_handlers")
+ @patch("airflow.settings.scoped_session")
+ @patch("airflow.settings.sessionmaker")
+ @patch("airflow.settings.create_engine")
+ def test_configure_orm_sqlite_file_based_gets_pool_settings(
+ self,
+ mock_create_engine,
+ mock_sessionmaker,
+ mock_scoped_session,
+ mock_setup_event_handlers,
+ monkeypatch,
+ ):
+ """SQLAlchemy 2.0+ uses QueuePool for file-based SQLite, so pool
settings should be applied."""
+ monkeypatch.setattr(settings, "SQL_ALCHEMY_CONN",
"sqlite:////tmp/airflow.db")
+ settings.configure_orm()
+ expected_kwargs = dict(
+ connect_args={"check_same_thread": False},
+ max_overflow=10,
+ pool_pre_ping=True,
+ pool_recycle=1800,
+ pool_size=5,
+ future=True,
+ )
+ mock_create_engine.assert_called_once_with(
+ "sqlite:////tmp/airflow.db",
+ **expected_kwargs,
+ )
+
+ @pytest.mark.parametrize("conn_str", ["sqlite://", "sqlite:///:memory:"])
+ def test_prepare_engine_args_sqlite_in_memory_skips_pool_settings(self,
conn_str, monkeypatch):
+ """In-memory SQLite uses SingletonThreadPool which doesn't support
pool_size/max_overflow."""
+ monkeypatch.setattr(settings, "SQL_ALCHEMY_CONN", conn_str)
+ engine_args = settings.prepare_engine_args()
+ assert "pool_size" not in engine_args
+ assert "max_overflow" not in engine_args
+ assert "pool_recycle" not in engine_args
Review Comment:
The test claims in-memory SQLite “skips pool settings”, but it doesn’t
assert that `pool_pre_ping` is absent (only
pool_size/max_overflow/pool_recycle). Since `pool_pre_ping` is also part of the
pool settings being skipped, add an assertion for it so the test fully covers
the intended behavior.
```suggestion
assert "pool_recycle" not in engine_args
assert "pool_pre_ping" not in engine_args
```
--
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]
