Hi Piyush, You asked for concrete points rather than a general "ripple effects" concern - fair ask. I dug into the code on current main and into the comments on the AIP page, and here is where I landed. My -1 stands, but these are the specific things that would change it.
1. Rollback only works for versions that were already pinned Christos' comment on the page is, I think, the most important open issue, and it is actually broader than he described. When a re-parse produces the same serialized Dag hash, write_dag does not create a new DagVersion - it overwrites the latest one's bundle_version in place, even when that version already has task instances: https://github.com/apache/airflow/blob/5428b3b995bcc94aea572a03fc35dc8a82fd2c15/airflow-core/src/airflow/models/serialized_dag.py#L704-L722 https://github.com/apache/airflow/blob/5428b3b995bcc94aea572a03fc35dc8a82fd2c15/airflow-core/tests/unit/models/test_serialized_dag.py#L715-L753 This came in with https://github.com/apache/airflow/pull/68336 (June), after the AIP was first written against 3.2.0. Existing runs are fine - they execute dag_run.bundle_version, which is frozen at run creation - but the DagVersion rows, which is what the AIP pins, no longer carry the history of code-only changes. I reproduced this locally with real git commits: after a code-only commit B, the DagVersion that runs from commit A were created on now reports commit B (the UI "view source" link for those runs points at B), and triggering a run at commit A returns 404. Details and the full log are in https://github.com/apache/airflow/issues/73861. The practical consequence: the most common bad deployment - a change in a task body, not in the Dag structure - leaves nothing to roll back to, because the "known good" DagVersion now points at the bad commit. The pin guard only freezes a version while it is pinned. So "instant rollback", one of the two headline use cases, works only for teams that run with a standing pin at all times. The AIP should either say that explicitly and present the standing pin as the operating model, or decide how code-only changes get recorded as versions for pinnable bundles (for example by not refreshing in place for them, or with some form of the commit-based pin Christos proposed). 2. Relation to "trigger with bundle_version" https://github.com/apache/airflow/pull/61550 (July) already lets you trigger a run at a given bundle_version. It resolves the version by looking up a DagVersion row with that bundle_version, so it has exactly the same gap - it fails with DagVersionNotFound for any commit that was refreshed in place: https://github.com/apache/airflow/blob/5428b3b995bcc94aea572a03fc35dc8a82fd2c15/airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py#L785-L827 It is also an existing one-shot "run the old version" primitive that overlaps with the rollback goal. I would like the AIP to explain how the two relate, and ideally fix the version lookup once for both. 3. Divergence detection needs a home There is already a static checker for runtime-varying values (dag_version_inflation_checker.py) that emits the RUNTIME_VARYING_VALUE Dag warning, and the Dag processor reconciles that warning type on every parse: https://github.com/apache/airflow/blob/5428b3b995bcc94aea572a03fc35dc8a82fd2c15/airflow-core/src/airflow/dag_processing/collection.py#L488-L492 The runtime divergence is observed on the worker, which cannot write to the DB. So the AIP needs to say which component detects it, how it is reported (that means a new Execution API call), and which warning type it uses - reusing RUNTIME_VARYING_VALUE would collide with the static checker and get wiped on the next parse. 4. Open review threads Ash's two comments (gating as a non-goal, and scheduler vs. parser owning the version choice) and Christos' two comments are still unanswered by them. Resolving threads on the page is fine, but I'd like to see their agreement, or the disagreement and the reasoning written into the AIP, before the next vote. 5. Two more from checking the code - Permissions: pinning goes through PATCH /dags, which only needs the same Dag "edit" permission as pause/unpause. Choosing which code runs is a Dag-author-level capability in our security model, so pin/unpin needs its own permission and an entry in security_model.rst. - db clean: the dag table is cleaned by last_parsed_time and cascades to dag_version https://github.com/apache/airflow/blob/5428b3b995bcc94aea572a03fc35dc8a82fd2c15/airflow-core/src/airflow/utils/db_cleanup.py#L220-L225 so a pinned Dag that stops being parsed would be deleted along with its pinned version. Also, dag_run.created_dag_version_id is ON DELETE SET NULL, so runs do not keep versions alive - only task instances do. 6. Target release Thanks for confirming 3.5.0 is fine. Please put that in the AIP so the vote is explicitly not about 3.4.0 scope. If 1-3 and 5 are addressed in the document and 4 is settled, I'm happy to re-review. J. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
