mwojtyczka commented on code in PR #63611:
URL: https://github.com/apache/airflow/pull/63611#discussion_r2938940966
##########
providers/databricks/src/airflow/providers/databricks/hooks/databricks_base.py:
##########
Review Comment:
The entire k8s call chain it invokes still accesses self.databricks_conn
synchronously
```
_a_get_token() ← fixed: uses adatabricks_conn()
└─ _a_get_federated_databricks_token()
├─ _get_required_client_id() ← sync, accesses self.databricks_conn
└─ _a_get_k8s_jwt_token() ← async, accesses
self.databricks_conn
├─ _a_get_k8s_projected_volume_token() ← async, accesses
self.databricks_conn
└─ _a_get_k8s_token_request_api() ← async, accesses
self.databricks_conn
```
So anyone using federated_k8s auth with a deferrable operator would still
hit the same AsyncToSync-in-event-loop crash that motivated this PR.
The fix would need to add async version of _get_required_client_id that
awaits adatabricks_conn().
Note: the existing test_no_sync_get_connection test doesn't catch this
because it uses Connection(login="foo", password="bar") which doesn't exercise
the federated_k8s branch. After the k8s methods are fixed, the regression test
should also exercise that path to prove it doesn't touch self.databricks_conn.
--
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]
