-
Notifications
You must be signed in to change notification settings - Fork 16.4k
AIP-72: Add support for getting connections from BaseHook using task sdk #47048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AIP-72: Add support for getting connections from BaseHook using task sdk #47048
Conversation
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
|
Hi @amoghrajesh, this PR seems to have affected some existing functionality in the AWS hook, specifically the boto credential fallback implemented here: airflow/providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py Lines 182 to 192 in 4002d83
Earlier, if you didn't have an aws connection, this code would create a boto session for the user but now it looks like this code block is never reached and instead the task fails due to not having an aws connection. It would be great if you could take a look at this, and let me know if you need anything else from me! |
|
Hey @ramitkataria, it is not easy for us to track each provider and each usage when such custom behaviour is implemented in inheriting hooks. From your justification, seems like you know the cause of the error, could you try and potentially fix it too?
|
|
Yep, makes sense - I was just hoping maybe you might know a potential cause for this. I spent some time earlier trying to figure out the cause but I'm currently not very familiar with TaskSDK. I'm planning to familiarize myself soon and then try debugging this again.
|
…ricHook` After apache#47048, the boto fallback strategy started breaking in `AwsGenericHook` (explained in more detail here: apache#47048 (comment)). I realized that this was because a new exception `AirflowRuntimeError` was introduced in `BaseHook`'s `get_connection`. It used to only throw `AirflowNotFoundException` earlier. This has been temporarily resolved in apache#47593 but to avoid issues once it does get re-introduced, `AwsGenericHook` will now also catch `AirflowRuntimeError`.
…ricHook` After apache#47048, the boto fallback strategy started breaking in `AwsGenericHook` (explained in more detail here: apache#47048 (comment)). I realized that this was because a new exception `AirflowRuntimeError` was introduced in `BaseHook`'s `get_connection`. It used to only throw `AirflowNotFoundException` earlier. This has been temporarily resolved in apache#47593 but to avoid issues once it does get re-introduced, `AwsGenericHook` will now also catch `AirflowRuntimeError`.
closes: #45233
Why?
#46869 introduced a way to get variables in task execution as well as the dag parsing time. We need to extend this support additionally to connections as well.
What?
Testing
DAG1: BaseHook.get_connection at task level
Logs:
DAG2: BaseHook.get_connection at DAG level
In addition to this, we can now also use
Connectionobject to get conneciton fromget:DAG:
Logs:
^ 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 newsfragments.