Move BaseHook class to task SDK#51873
Merged
amoghrajesh merged 66 commits intoapache:mainfrom Jul 2, 2025
Merged
Conversation
amoghrajesh
requested review from
Fokko,
ashb,
bolkedebruin,
eladkal,
gopidesupavan,
hussein-awala,
jedcunningham,
jscheffl,
kaxil,
mobuchowski,
o-nikolas and
potiuk
as code owners
boring-cyborg
bot
added
area:CLI
area:core-operators
area:DAG-processing
area:lineage
area:providers
area:serialization
area:Triggerer
kind:documentation
provider:airbyte
provider:alibaba
provider:amazon
Contributor
Author
|
Green again finally!! |
kaxil
approved these changes
Jul 1, 2025
Member
There was a problem hiding this comment.
Let's create a GIthub issue to resolve Connection thing to have real sdk.Connection and for #51873 (comment) and #51873 (comment)
1 task
Contributor
Author
|
Thanks for the review @kaxil! Here are the issues which I will work on as follow ups: |
Contributor
|
All of he AWS system tests have been failing since this commit merged. I reverted this one and #52766 locally to make sure and they pass with this reverted. I'm looking into it, but if any cause comes to mind, let me know. |
ramitkataria
added a commit
to aws-mwaa/upstream-to-airflow
that referenced
this pull request
Jul 4, 2025
…OUND` Task SDK exception After apache#51873, the base hook's `get_connection` exception for when connection is not found was changed and was no longer being caught by `AwsGenericHook`. This change fixes that by using a robust approach. At some point, we should probably make the connection getter raise exceptions in a more consistent manner, maybe once we fully switch over to Task SDK.
Contributor
|
The cause of this error was a change in the string representation of the exception after this PR was merged. In #52838, I changed it to a what seems like a more robust way of catching the "connection not found" exception. But I think at some point, we should probably make the connection getter raise exceptions in a more consistent manner (maybe once we fully switch over to Task SDK) so that callers of that method can have a simple try/except to handle the not found case |
Contributor
Author
Hey @ramitkataria what string representation changed? Could you elaborate a bit there? |
ferruzzi
pushed a commit
that referenced
this pull request
Jul 4, 2025
…OUND` Task SDK exception (#52838) After #51873, the base hook's `get_connection` exception for when connection is not found was changed and was no longer being caught by `AwsGenericHook`. This change fixes that by using a robust approach. At some point, we should probably make the connection getter raise exceptions in a more consistent manner, maybe once we fully switch over to Task SDK.
Contributor
|
@amoghrajesh I have seen strange plugin initialization errors in https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1751744572678519 - is this related to this PR? |
Contributor
|
@amoghrajesh Also antoher thing, maybe related? Started breeze on main via Is this related to this PR? Or shall I open a separate issue ticket? |
Contributor
Author
|
@jscheffl could be related. Can you please create issues(either for one for both)? I can only look on Monday, wont be around tomorrow |
Contributor
|
2 tasks
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes: #51672
Moving the BaseHook class into task SDK, exactly where it should live similar to other base classes in sdk like notifiers, operators etc.
Testing: Running by all the methods defined on BaseHook -- old vs new
Running with the new path
DAG:
Running with the older path to check backcompat
^ 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.