Thanks for the review, Jarek. I've responded in the discussion thread below to keep discussions consolidated. https://lists.apache.org/thread/6h811nmzjrgfhj1b0kwqtjjlhvc5jrvr
I'll start a fresh voting thread later once we finish this set of reviews. Thanks. On Tue, Sep 29, 2026 at 12:05 AM Jarek Potiuk <[email protected]> wrote: > 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] > >
