pierrejeambrun commented on code in PR #67900:
URL: https://github.com/apache/airflow/pull/67900#discussion_r3461133503
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/backfills.py:
##########
@@ -162,6 +163,7 @@ def unpause_backfill(backfill_id: NonNegativeInt, session:
SessionDep) -> Backfi
raise HTTPException(status.HTTP_409_CONFLICT, "Backfill is already
completed.")
if b.is_paused:
b.is_paused = False
+ session.commit()
Review Comment:
Why committing here? `session` dep is committing on exit.
##########
airflow-core/src/airflow/models/backfill.py:
##########
@@ -628,7 +628,15 @@ def _create_backfill(
if not serdag:
raise DagNotFound(f"Could not find dag {dag_id}")
- dag_model = session.scalar(select(DagModel).where(DagModel.dag_id ==
dag_id).limit(1))
+ # Lock the DagModel row (FOR UPDATE on PG/MySQL) to serialize
concurrent
+ # backfill creation for the same dag_id. This prevents two requests
from
+ # both passing the num_active check before either commits.
+ dag_model = session.scalar(
+ with_row_locks(
+ query=select(DagModel).where(DagModel.dag_id ==
dag_id).limit(1),
+ session=session,
+ )
Review Comment:
I wouldn't add this lock just to prevent a sqlite potential issue. Deadlock
for production db are a concern and I think the handling added above is enough
since sqlite isn't production ready.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_backfills.py:
##########
@@ -353,6 +354,67 @@ def test_create_backfill_with_depends_on_past(
== "Dag has tasks for which depends_on_past=True. You must
set reprocess behavior to reprocess completed or reprocess failed."
)
+ def test_create_backfill_database_locked(self, session, dag_maker,
test_client):
+ """Regression test for issue #66726: SQLite 'database is locked'
returns 503."""
Review Comment:
Don't put such docstring `regression test for issue #66726:` in tests. As
per our contribution guidelines this should just describe what the test does.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_backfills.py:
##########
@@ -353,6 +354,67 @@ def test_create_backfill_with_depends_on_past(
== "Dag has tasks for which depends_on_past=True. You must
set reprocess behavior to reprocess completed or reprocess failed."
)
+ def test_create_backfill_database_locked(self, session, dag_maker,
test_client):
Review Comment:
With this current implementation, I don't think this test will only run when
running the test suite for `sqllite`. You need a db marker here to skip it on
mysql and postgres I think.
--
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]
