kaxil commented on PR #74050: URL: https://github.com/apache/airflow/pull/74050#issuecomment-5939443752
Thanks for the PR, but I'm closing it because it doesn't show that it fixes anything. #59120 is about PostgreSQL: once a DB error aborts the transaction inside the `_create_dag_runs` loop, the `except Exception: ... continue` can't recover without a rollback or a savepoint. The description says the tests were only run on SQLite and "do not reproduce PostgreSQL's aborted-transaction behavior", which is the exact behavior the issue is about. So neither you nor a reviewer has evidence this change helps. A few other problems: - The scheduler loop isn't touched. The diff only relaxes `CommitProhibitorGuard` so it ignores savepoint releases. That might be one piece of a savepoint-based fix, but on its own it doesn't change how `_create_dag_runs` handles a failed dag run. - The description mentions a passing "scheduler regression test", but no scheduler test is in the diff. The only new test runs `SELECT 1` in a savepoint and doesn't cover the scenario from the issue. - The new lines in `_validate_commit` use 9/10-space indentation, so this wouldn't pass `ruff-format`. If you want to take another run at #59120, please: 1. Write a test that fails on `main` against PostgreSQL (`breeze testing core-tests --backend postgres`, or `breeze run --backend postgres pytest ...`), with a DB error raised partway through `_create_dag_runs`. 2. Fix the loop itself (a savepoint per dag run or a smaller transaction per dag run, as the issue suggests), and change the guard only if the fix needs it. 3. Show that the test passes on PostgreSQL with the fix. -- 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]
