Vamsi-klu commented on PR #71752: URL: https://github.com/apache/airflow/pull/71752#issuecomment-5343646150
> Thanks — the Phase 1 contract is preserved carefully here, and the state-machine edge cases we went through on #70088 (stale `STOPPED` not terminal, deletion mid-wait, the deadline boundary) are all covered by deterministic tests rather than sleeps. > > I validated the deferrable paths against a real workspace: Airflow 3.2.2, this branch's provider installed from source, a dedicated throwaway serverless warehouse (2X-Small), triggerer doing the waiting. > > | scenario | task state | evidence | > | --- | --- | --- | > | deferrable start from `STOPPED` | deferred -> success | trigger logged `is STARTING; waiting for RUNNING`, warehouse reached `RUNNING` | > | deferrable stop | deferred -> success | warehouse reached `STOPPED` | > | start against an already-`RUNNING` warehouse | success, never deferred | no `DeferTask` for the task | > | `wait_for_termination=False` stop | success, never deferred | transition still requested, warehouse stopped afterwards | > | `timeout=1` start | deferred -> failed | `DatabricksWarehouseError: ... did not reach RUNNING within 1s; last state: STARTING.` | > > So the behavioural claims in the description hold up where I could exercise them. Unit tests reproduce your numbers as well (43 + 14 + 13, and the full provider suite at 922 passed / 12 skipped). > > Two things I would like settled before merge, both left inline: > > - The changelog entry is inserted under the `7.18.1` header, which is an already-released version, so a shipped release would advertise a feature it does not contain. Per `providers/AGENTS.md` and the `NOTE TO CONTRIBUTORS` block at the top of that file, routine feature entries are collected by the release manager from commit messages anyway — the five lines can just go. > - The deferrable `timeout` is a duration recomputed inside every `run()`, so it restarts from zero on each triggerer restart or HA rebalance, while the synchronous path fails deterministically. `DatabricksSQLStatementExecutionTrigger` next door serializes an absolute `end_time` for exactly this reason. Either follow that pattern or state the per-run semantics explicitly — right now the docstring and the PR description claim the opposite of what the code does. > > The rest is nits, also inline. One out-of-scope note worth being aware of rather than fixing here: these operators' first deferrable surface inherits #71525, so on core 3.0.x the trigger cannot fetch the connection at all (3.1.0+ is fine). Whether that caveat belongs in this page or centrally is a call for the docs discussion happening on #71667. > Thanks for the live workspace check. The follow-up is on the branch. Changelog Features under 7.18.1 is gone. The trigger now serializes an absolute end_time the same way DatabricksSQLStatementExecutionTrigger does. a_get_warehouse shares the GET path with get_warehouse. The on_kill override and the unpinned test are gone. The typed state JSON stays for sibling trigger symmetry, and the description now mentions the Phase 1 to_json restore. The 3.0.x connection fetch caveat from 71525 is still out of scope here. -- 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]
