sean-rose commented on PR #48780: URL: https://github.com/apache/airflow/pull/48780#issuecomment-2816418686
> This is not right the right place to do this -- functions that take a session should not commit, that is either handled by the decorator, or should be the responsibility of the caller. It would be ideal if the session commit could be handled by the decorator or caller, but in the scenario we're running into that isn't happening successfully, so IMO Airflow ought to be more proactive in persisting the task instance history records. Also, from looking at other Airflow code it doesn't seem uncommon for functions that take a session to commit (e.g. there are eight such `session.commit()` calls in [`airflow/models/taskinstance.py`](https://github.com/sean-rose/airflow/blob/a61248878939891d6d43dc1a8e3c6c10c7733a91/airflow/models/taskinstance.py), all within functions decorated with `@provide_session`). > So this commit needs to be handled somewhere else The `TaskInstanceHistory.record_ti()` call that isn't successfully persisting the record in our case is [in `TaskInstance.fetch_handle_failure_context()`](https://github.com/sean-rose/airflow/blob/a61248878939891d6d43dc1a8e3c6c10c7733a91/airflow/models/taskinstance.py#L3383), so one option would be adding a `session.commit()` call there (though that method is also decorated with `@provide_session`). Or we could create a separate session there specifically for saving the task instance history record, like: ```python with create_session() as ti_history_session: TaskInstanceHistory.record_ti(ti, session=ti_history_session) ``` However, I'm not familiar enough with the Airflow codebase to know what drawbacks creating a session like that might have. I generally try to follow the conventions/precedents of the surrounding code when making changes in unfamiliar codebases, and at least in `airflow/models/taskinstance.py` calling `session.commit()` seemed like the more common convention than explicitly creating new sessions. What approach would you recommend? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
