You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/25 18:48:57 UTC
[GitHub] [airflow] potiuk opened a new pull request, #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
potiuk opened a new pull request, #25293:
URL: https://github.com/apache/airflow/pull/25293
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
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1194600768
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 :).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Allow Legacy SqlSensor to use the common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1197425291
Yep. Previous merged already.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Allow Legacy SqlSensor to use the common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1195196047
Rebased on top of #25299 so only last commit (very small) matters
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1194489512
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`
?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1195015154
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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1195019959
> 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 ).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1194710714
PR here #24836
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1194483298
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:
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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25293: Fix legacy Airflow SqlSensor rejecting common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25293:
URL: https://github.com/apache/airflow/pull/25293#issuecomment-1195191281
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).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #25293: Allow Legacy SqlSensor to use the common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25293:
URL: https://github.com/apache/airflow/pull/25293
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org