Fix consistency in exception/log handling in connection.py
#50329
Conversation
In one of my previus PRs (apache#47648), I incorrectly assumed that catching the exception from `secrets_backend.get_connection` was meant for the connection not found case. So, I'm reversing a couple of changes from that PR for consistency.
amoghrajesh
left a comment
amoghrajesh
left a comment
There was a problem hiding this comment.
Makes sense to do this, some nits
| return conn | ||
| except Exception: | ||
| log.debug( | ||
| log.exception( |
There was a problem hiding this comment.
Why is this logexception? It was correct before since we are looping through various secret backend here
There was a problem hiding this comment.
I originally changed this to log.debug in PR #47648 because I misunderstood the behaviour. secrets_backend.get_connection returns None for "not found" cases and throws exceptions only for actual errors as far as I can tell. I encountered an issue with fernet keys where not logging the exception hid the actual error, showing only "Unable to retrieve connection from secrets backend (%s). Checking subsequent secrets backend." without any stack trace or error details which made it appear like the connection was just missing.
Using log.exception ensures that when there's a genuine error accessing a secrets backend (not just a missing connection), we get the full error details for debugging. We could also just set exc_info to True and keep using log.debug but this seems important enough for most users to know in case they're running into an issue where the connection exists but airflow can't find it.
There was a problem hiding this comment.
@ramitkataria The one you changed in PR #47648 is the different one I am referring to here. You already removed log.debug line you had added in that PR in this PR on line 481. The one I linked here is the one that loops across all secrets backends
This was an error in apache#50329
…50329) * Fix consistency in exception/log handling in `connection.py` In one of my previus PRs (apache#47648), I incorrectly assumed that catching the exception from `secrets_backend.get_connection` was meant for the connection not found case. So, I'm reversing a couple of changes from that PR for consistency. * Change log message
This was an error in apache#50329
This was an error in apache/airflow#50329 GitOrigin-RevId: d52f8b65fb5ab065e8f214cd00563945aef6d29f
4 participants
In one of my previus PRs (#47648), I incorrectly assumed that catching the exception from
secrets_backend.get_connectionwas meant for the connection not found case. So, I'm reversing a couple of changes from that PR for consistency.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.