fritz-astronomer commented on PR #47082:
URL: https://github.com/apache/airflow/pull/47082#issuecomment-2683545324
@eladkal my thoughts:
- establishing/testing connections is something I see Airflow users struggle
with constantly, and it can often be pretty frustrating/obscure what is or
isn't working, especially for someone new to Airflow. Isolating just the
connection can simplify the process and should be encouraged and easy
- giving users the tools to spend less time debugging and make a difficult
process easy is useful
- the canary pattern wants to exercise the connection in a meaningful but
non-impactful way (e.g. `select 1;` not `select * from my_table limit ...`). It
isn't always immediately obvious how to do that, when establishing a connection
to a system that a user may only have partial familiarity with, or users may
make a disruptive test by accident
- this could also be a great `@setup` task, or something to encourage at
the start of a DAG, e.g. to see if the database actually is up and exists
- The functionality for `test_connection` is already there for many
hooks/connection types, and includes the correct tests, so I'd say "D.R.Y.".
Why make users reimplement a user-friendly feature?
- not all connections are in the Airflow UI (e.g. a secrets backend), so
this has additional benefit for testing those connections, and the ability to
retrieve them. The pattern also functions as a way to document what connections
exist and are functional for the Airflow instance
- that codepath is often disabled / unusable via the AF Connections UI.
Often users don't even know that exists, nor why it's disabled, nor how to
enable it or the implications of enabling it instance-wide. I'd say this is
good for security while preserving a friendly UX that had been previously
exposed
---
One change in this from the `test_connection` function. The original doesn't
actually fail, when invoked via a task, which is why this is modified to raise.
If `.test_connection` had the option to raise, an alternative could be
publishing/documenting:
```
@task
def canary(conn_id):
return BaseHook.get_hook(conn_id=self.conn_id).test_connection(raise=True)
```
(though my vote would still be "why expect new users to know this random
snippet, or find it in a doc or blog post?" Something off the shelf feels more
friendly and useful, especially given how connections are one of the most core
pieces of Airflow and are a pretty rough-edged UX at the moment)
--
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]
