ephraimbuddy commented on issue #71453: URL: https://github.com/apache/airflow/issues/71453#issuecomment-5263949691
I agree with the core problem: `created_dag_version_id` is documented as the version in effect when the Dag run was created, but the field is now used as the run's current version pointer. `DagRun.bundle_version` moves with it, so the two fields should be treated as one resolution mechanism. There are a few corrections that affect the proposed fix, though. First, #71425 does not introduce this mutation for queued or running Dag runs. `main` already updates `created_dag_version_id` and `bundle_version` in that path; #65835 introduced it on `main`, and #66901 backported it to 3.2.2. #71425 makes the existing behavior consistent: it moves a running task that becomes `RESTARTING`, runs `verify_integrity`, and applies the version update even when `dag_run_state=False`. Second, the #71455 repro as written does not reproduce on the finished-run path. That path already calls `verify_integrity`, which creates the TI for newly added task C during the first clear. The subsequent `only_new` result is therefore correctly empty. There is a reachable variant on current `main`: the queued/running branch moves the run's pointer without calling `verify_integrity`, so the pointer can say latest while the corresponding TI does not exist. #71425 fixes that variant. Third, #71454 is valid and is not limited to the Grid chips. `DagRun.dag_versions` also feeds the public `DAGRunResponse` and `DagRun.version_number`. That should be fixed independently by reporting the versions represented by the run's TI/TIH rows instead of assuming a bundle-versioned run can only contain the run's current pointer. On the two design questions: 1. **Keep run-on-latest for versioned bundles.** It has been a released user-facing capability since 3.1.0 and now exists across the clear UI/API, backfills, `[core] rerun_with_latest_version`, and the Dag-level parameter. The API still marks it experimental, but removing the capability would nevertheless be a separate user-visible design and compatibility decision; it should not be folded into #71425. Disabling bundle versioning is not equivalent: users can reasonably want runs pinned by default while deliberately rerunning a failed task against fixed code. 2. **Do not add a second column without a concrete provenance consumer.** The execution-path consumers I found use this value as the version the run currently resolves to. Reporting consumers such as `dag_versions`, and task-existence logic such as `_get_new_task_ids`, should derive their answers from TI/TIH rows instead. An additional immutable column would add a migration, dual-write rules, and a new choice at every reader. It would also not restore the documented contract for existing rows reliably: `_update_dagrun_to_latest_version` can rewrite all current TI version ids without creating TI history, so the original creation version may already be unrecoverable. If immutable creation-time provenance has a real consumer, we should define its retention and upgrade semantics first. I could not find such a consumer in the current code. My suggested split is: 1. Land #71425 on its own merits; it makes the already-released behavior internally consistent and fixes the reachable queued/running variant of #71455. 2. Re-document `created_dag_version_id` as the Dag version the run currently resolves to, initially set at creation and updated when the run is explicitly moved to latest. Discuss a physical rename separately for 3.4. 3. Fix #71454 from the TI/TIH source of truth so mixed-version runs report all versions actually in use. 4. Change `_get_new_task_ids` to compare the latest Dag's task ids with the run's existing TI rows. That directly answers which tasks are missing and removes its dependency on the version-pointer invariant. I am open to a separate immutable field if there is a concrete consumer for creation-time provenance, but I do not think the current issue establishes one. I agree with the core problem: `created_dag_version_id` is documented as the version in effect when the Dag run was created, but the field is now used as the run's current version pointer. `DagRun.bundle_version` moves with it, so the two fields should be treated as one resolution mechanism. There are a few corrections that affect the proposed fix, though. First, #71425 does not introduce this mutation for queued or running Dag runs. `main` already updates `created_dag_version_id` and `bundle_version` in that path; #65835 introduced it on `main`, and #66901 backported it to 3.2.2. #71425 makes the existing behavior consistent: it moves a running task that becomes `RESTARTING`, runs `verify_integrity`, and applies the version update even when `dag_run_state=False`. Second, the #71455 repro as written does not reproduce on the finished-run path. That path already calls `verify_integrity`, which creates the TI for newly added task C during the first clear. The subsequent `only_new` result is therefore correctly empty. There is a reachable variant on current `main`: the queued/running branch moves the run's pointer without calling `verify_integrity`, so the pointer can say latest while the corresponding TI does not exist. #71425 fixes that variant. Third, #71454 is valid and is not limited to the Grid chips. `DagRun.dag_versions` also feeds the public `DAGRunResponse` and `DagRun.version_number`. That should be fixed independently by reporting the versions represented by the run's TI/TIH rows instead of assuming a bundle-versioned run can only contain the run's current pointer. On the two design questions: 1. **Keep run-on-latest for versioned bundles.** It has been a released user-facing capability since 3.1.0 and now exists across the clear UI/API, backfills, `[core] rerun_with_latest_version`, and the Dag-level parameter. The API still marks it experimental, but removing the capability would nevertheless be a separate user-visible design and compatibility decision; it should not be folded into #71425. Disabling bundle versioning is not equivalent: users can reasonably want runs pinned by default while deliberately rerunning a failed task against fixed code. 2. **Do not add a second column without a concrete provenance consumer.** The execution-path consumers I found use this value as the version the run currently resolves to. Reporting consumers such as `dag_versions`, and task-existence logic such as `_get_new_task_ids`, should derive their answers from TI/TIH rows instead. An additional immutable column would add a migration, dual-write rules, and a new choice at every reader. It would also not restore the documented contract for existing rows reliably: `_update_dagrun_to_latest_version` can rewrite all current TI version ids without creating TI history, so the original creation version may already be unrecoverable. If immutable creation-time provenance has a real consumer, we should define its retention and upgrade semantics first. I could not find such a consumer in the current code. My suggested split is: 1. Land #71425 on its own merits; it makes the already-released behavior internally consistent and fixes the reachable queued/running variant of #71455. 2. Re-document `created_dag_version_id` as the Dag version the run currently resolves to, initially set at creation and updated when the run is explicitly moved to latest. Discuss a physical rename separately for 3.4. 3. Fix #71454 from the TI/TIH source of truth so mixed-version runs report all versions actually in use. 4. Change `_get_new_task_ids` to compare the latest Dag's task ids with the run's existing TI rows. That directly answers which tasks are missing and removes its dependency on the version-pointer invariant. I am open to a separate immutable field if there is a concrete consumer for creation-time provenance, but I do not think the current issue establishes one. --- Drafted-by: Codex (5.6 Sol); reviewed by @ephraimbuddy before posting -- 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]
