viiccwen commented on code in PR #69988:
URL: https://github.com/apache/airflow/pull/69988#discussion_r3600824706
##########
airflow-core/tests/unit/jobs/test_triggerer_job.py:
##########
Review Comment:
Please expand the test to cover both pinned and unpinned cases and set
`dag_run.bundle_version` explicitly.
Also, please add `spec`/`autospec` to the new mocks and remove `assert
run_version != umped_ti_version`; that assertion tests `uuid.uuid4()` rather
than behavior introduced by this PR.
##########
airflow-core/tests/unit/jobs/test_triggerer_job.py:
##########
@@ -609,6 +609,48 @@ def
test_create_workload_uses_supervisor_id_without_job(jobless_supervisor, mock
assert factory.log_path == f"/logs/ti.trigger.{jobless_supervisor.id}.log"
+def
test_create_workload_resolves_serialized_dag_from_run_created_version(jobless_supervisor,
mocker):
+ """If the run and the task point at different DAG versions, the trigger
should load the run's version."""
Review Comment:
It should be "Dag", not "DAG".
##########
airflow-core/src/airflow/jobs/triggerer_job_runner.py:
##########
@@ -876,7 +876,8 @@ def _create_workload(
)
serialized_dag_model = dag_bag.get_serialized_dag_model(
- version_id=trigger.task_instance.dag_version_id,
+
version_id=trigger.task_instance.get_dagrun(session=session).created_dag_version_id
+ or trigger.task_instance.dag_version_id,
Review Comment:
`created_dag_version_id` is populated even for Dags with
`disable_bundle_versioning=True`; `dag_run.bundle_version` is what determines
whether the run is pinned. This unconditional preference for
`created_dag_version_id` therefore makes an unpinned `start_from_trigger` task
load the Dag version from when the run was created, even after the scheduler
has advanced the unfinished TI's `dag_version_id` following a reparse.
This differs from `DBDagBag._version_from_dag_run()`, which intentionally
resolves the latest version when `bundle_version` is absent. Please use the
run's `created_dag_version_id` only when the run is pinned, retain the TI
version for unpinned runs, and cover both cases in the regression test.
--
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]
