Skip to content
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

Allow Legacy SqlSensor to use the common.sql providers #25293

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 25, 2022

The legacy Airflow SqlSensor, rejects working wiht comon.sql providers
eve if they are perfectly fine to use.

While users could switch to the common.sql sensor (and it
should work fine). we should not force the users to switch to it.

We are implementing "fake" class hierarchy in case the provider
is installed on Airflow 2.3 and below.

In case of Airflow 2.4+ importing the old DbApiHook will fail,
because it will cause a circular import - in such case our
new DbApiHook will derive (as it was originally planned) from
BaseHook.

But In case of Airflow 2.3 and below such import will succeed
and we are using the original DbApiHook from airflow.hooks.dbapi
as base class - this way any "common.sql" hook on Airflow 2.3
and below will also derive from the airlfow.hooks.dbapi.DbApiHook

  • thus it will be possible to use it by the original SqlSensor.

Fixes: #25274


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Jul 25, 2022

This is a very interesting and originally unforeseen effect of common.sql provider separation,

It's hard to test it with unit tests, because you actually you need install Airflow 2.3 or below to test it (so the tests that I added are not actually testing what you might think they are)

But I tested it manually:

  • I prepared new providers (common.sql and oracle)
  • I run 2.3.3 "bare version" of airflow and installed the common.sql and oracle
  • I defined oracle connection 'test_oracle'

Then I repeated what "SqlSensor" does from Airflow 2.3.3:

image

After this change when common.sql DbApiHook is installed on Airlfow 2.4+ it inherits from BaseHook, but when installed in Airlfow 2.3, it inherits from airflow.hooks.dbapi.DbApiHook (it overrides all the legacy hook methods so this has virtually no side effect).

This allows the airflow.sensors SqlSensor to use the new Hook (which is returned by get_hook() on 'Connection' , so the SqlSensor does not know it comes from the new provider.

@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch from 22b873d to 560d334 Compare July 25, 2022 18:59
@eladkal
Copy link
Contributor

eladkal commented Jul 25, 2022

You mentioned the issue is due to
from airflow.sensors import SqlSensor
So i guess this problem is not unique to SqlSensor and also happens for any core operators that was replaced by providers if imported as
from airflow.operators import X
?

NVM missread the description

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm what about BranchSqlOperator?
I would expect it to have the same issue.
Or we didn't move it from core to the provider yet?

@potiuk
Copy link
Member Author

potiuk commented Jul 25, 2022

Actually ... yeah. We haven't moved them yet :).. This is a good point that we should have ... But the change will work fine for all kinds of combinations :).

@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch from 560d334 to e798a60 Compare July 25, 2022 20:47
@potiuk
Copy link
Member Author

potiuk commented Jul 25, 2022

PR here #24836

@uranusjr
Copy link
Member

Maybe instead of importing the deprecated base class, we should do different things depending on Airflow version:

  1. If running on Airflow < 2.3, import the legacy base class.
  2. In Airflow 2.3+, we add additional logic in BaseSQLOperator to also accept hooks inheriting the new base class.

This would avoid needing the filterwarning block, which feels like a hack to me.

@potiuk
Copy link
Member Author

potiuk commented Jul 26, 2022

Maybe instead of importing the deprecated base class, we should do different things depending on Airflow version:

  1. If running on Airflow < 2.3, import the legacy base class.
  2. In Airflow 2.3+, we add additional logic in BaseSQLOperator to also accept hooks inheriting the new base class.

This would avoid needing the filterwarning block, which feels like a hack to me.

Yeah. I was wondering if it's not too much of a hack myself. I think we already use version check in a number of places and I agree using it here might be a better idea (I also do not know performance implications of the circular import detection ).

@potiuk
Copy link
Member Author

potiuk commented Jul 26, 2022

I will need #25299 before we merge this one, otherwise we have far too many mypy problems to solve (and it is good for upcoming unification of methods in common.sql).

@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch from e798a60 to 2ff791c Compare July 26, 2022 08:49
@potiuk potiuk requested a review from turbaszek as a code owner July 26, 2022 08:49
@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch from 2ff791c to 0f6c296 Compare July 26, 2022 08:50
@potiuk potiuk changed the title Fix legacy Airflow SqlSensor rejecting common.sql providers Allow Legacy SqlSensor to use the common.sql providers Jul 26, 2022
@potiuk
Copy link
Member Author

potiuk commented Jul 26, 2022

Rebased on top of #25299 so only last commit (very small) matters

@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch 2 times, most recently from e5bd9aa to 058ae19 Compare July 26, 2022 11:27
@potiuk potiuk requested review from ashb and mik-laj as code owners July 26, 2022 11:27
@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch from 058ae19 to 40d732e Compare July 26, 2022 11:36
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the one commit specifically.

The legacy Airflow SqlSensor, rejects working wiht comon.sql providers
eve if they are perfectly fine to use.

While users could switch to the common.sql sensor (and it
should work fine). we should not force the users to switch to it.

We are implementing "fake" class hierarchy in case the provider
is installed on Airflow 2.3 and below.

In case of Airflow 2.4+ importing the old DbApiHook will fail,
because it will cause a circular import - in such case our
new DbApiHook will derive (as it was originally planned) from
BaseHook.

But In case of Airflow 2.3 and below such import will succeed
and we are using the original DbApiHook from airflow.hooks.dbapi
as base class - this way any "common.sql" hook on Airflow 2.3
and below will also derive from the airlfow.hooks.dbapi.DbApiHook
- thus it will be possible to use it by the original SqlSensor.

Fixes: apache#25274
@potiuk potiuk force-pushed the fix-legacy-sql-check-works-with-new-providers branch from 40d732e to 3b44a7f Compare July 27, 2022 20:05
@potiuk
Copy link
Member Author

potiuk commented Jul 27, 2022

Yep. Previous merged already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apache Airflow SqlSensor DbApiHook Error
3 participants