moomindani commented on issue #71525: URL: https://github.com/apache/airflow/issues/71525#issuecomment-5555610421
Four PRs now point at this issue — #71840, #72014, #72077, #72245 — and only one has had a review, so here is the comparison in one place. I am not a committer; this is the evidence, not a decision. The constraint that decides most of it: the crash only happens on core **3.0.x** ([version matrix](https://github.com/apache/airflow/issues/71525#issuecomment-5302280351)), and the provider supports `apache-airflow>=2.11.0`. `BaseHook.aget_connection` first ships in **3.1.0** (added in `b83fcf9a31`; the earliest release tag containing it is 3.1.0). A fix built on `aget_connection` alone therefore only runs on the versions where the bug is already gone. | PR | how it gets the connection | works on 3.0.x | tests | CI | |---|---|---|---|---| | #71840 | `get_async_connection(..., hook=self)` from common.compat — native `aget_connection` on 3.1+, `sync_to_async` fallback below | yes | yes | red, see below | | #72245 | the same compat helper | yes | none in the diff | not run (only Mergeable/WIP) | | #72077 | `sync_to_async(self.get_connection)` directly | yes | yes | not run (only Mergeable/WIP) | | #72014 | `BaseHook.aget_connection` | no | yes, but in `test_databricks.py` while the change is in `databricks_base.py` | red | **#71840 is the closest to mergeable**, and two small things stand between it and green CI: - `get_async_connection` is called at `databricks_base.py:201` but never imported. That single omission is the whole of the ruff and mypy failure (`Name "get_async_connection" is not defined`), and every downstream job fails on the same `NameError` rather than on anything about the approach. - the `hook=` keyword arrived with common.compat **1.17.0** (#69140), while the provider pins `apache-airflow-providers-common-compat>=1.13.0`. That bound needs raising — or drop `hook=` and lose the subclass override it exists to honour. **#72077**'s approach also works, and it carries no compat dependency, but it hardcodes the `sync_to_async` fallback and so never takes the native async path on 3.1+ — which is the choice the compat helper exists to make. **#72245** additionally adds `a_get_run_tasks`, `a_cancel_run` and `a_cancel_sql_statement`, which are unrelated to this crash and would be easier to review as their own PR. Whichever lands, #71667 documents the version floor and stays useful alongside it. --- Drafted-by: Claude Code (Opus 5); reviewed by @moomindani 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]
