zhengxiang378928908-code commented on PR #74658: URL: https://github.com/apache/airflow/pull/74658#issuecomment-6182448045
Good point, and thanks for pushing on it. Two things: **Wasted work when the DB is down:** this PR doesn't change that policy. Since #65313, `handle_parsing_result` already catches a failed persist, logs it, throttles that file and moves on to the others. The bug is that this path is never reached here: `update_dag_parsing_results_in_db` swallows the error, returns normally, and the failure only surfaces as `PendingRollbackError` from the `@provide_session` commit, which nothing catches. If the processor should rather exit after repeated DB failures, I'm happy to follow whatever you think is right, but that seems like a separate decision from this fix. **The retry decorator:** I agree it fits, but it can't catch this on its own, because the exception never reaches it: it's swallowed inside `update_dag_parsing_results_in_db` and only resurfaces at commit time, outside any retry wrapper. So I'd keep the re-raise for an unusable session, and then put `@retry_db_transaction` on `persist_parsing_result` in place of the manual `session.rollback()` in `handle_parsing_result`. A transient drop then gets rolled back and the whole persist retried right away, and only after the retries are exhausted does it fall through to the existing throttling path. I'd also keep the savepoints around the import-error and warning writes. `IntegrityError` is a `DBAPIError` too, so without them a deterministic failure in those best-effort writes would retry the whole persist and then drop the Dag updates with it. If that sounds right to you, I'll push it. -- 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]
