-
Notifications
You must be signed in to change notification settings - Fork 15k
Move SQS message queue to Amazon provider #50057
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
Move SQS message queue to Amazon provider #50057
Conversation
325abda
to
30c6f91
Compare
Few things that this PR leverages from our existing framework for provider's capabiliy discovery:
All that comes out-of-the-box after tapping-in the standard provider's discovery mechanism, BTW. The new And I would really appreciate some suggestions (or even fixups) to the documentation I created. I slapped things together quickly from AIP-82 and few other places, but I am pretty sure we can do way better and more "catchy" description - maybe even add some images from the AIP-82 design etc. |
30c6f91
to
bff12a2
Compare
That was rather straightforward and quick as I thought - I even added missing unit tests for the SQS provider. |
providers/common/messaging/src/airflow/providers/common/messaging/providers/__init__.py
Show resolved
Hide resolved
bff12a2
to
8a49397
Compare
I'd also appreciate some "real-life" testing @vincbeck - I do not want to setup all the SQS queue for me and I fixed all the unit tests (I think) - so I am quite positive it will work, but well ... you never know. |
8a49397
to
a2072e4
Compare
a2072e4
to
106cf78
Compare
If you look closely - our tests had shown that there are a couple of consequences of "fixing the common.messaging" . So I think it would be good to summarize it here. Also for @kaxil and @eladkal -> to agree and realise the impact it will have on Airflow and provider's releases:
That leaves a little bit of doubt - should we merge it to 3.0.1 ("bugfix") or should it be 3.1.0 ("feature") of airflow to discover common.messaging providers. I'd lean towards "bug-fix" and merging it with 3.0.1, in a way this is fixing the way how common.messaging works and Airflow 3.0.0 has a lot more things that all but guarantee literally no-one will use it. And we had a precedent for that - Airflow 2.0.0 had a bug in provider's manager interface that would prevent evolution of providers and we implemented a fix in 2.0.1 (or even 2.0.2 - can't remember) that fixed it by adding essentially new thing to airflow. This is not really a "user" facing feature, and intention is to "fix" things - so for me this is reeally a "fix" and merging it to 3.0.1 is something I would strongly recommend. That also will allow us to implement a lot more common.messaging interfaces way quicker without waiting for 3.1.0 (like the Kafka one) A small consequence of that is that we have to add `"common.messaging" to exclude it for comatibility matrix tests - for Airflow 3.0.0, but that's fine - and we will remove it from that matrix once we bump the compatibility tests to 3.0.1. I am also torn on 1.0.1 vs. 1.1.0 for common-messaging - It can go with either - we are far more relaxed about feature versions for providers. But I will leave it to @eladkal to decide. So all seems on a clear path to be cherry-picked to 3.0.1 I think (and the code changed in airflo is really a copy&paste /extension of the other provider manager's features, so that is rather safe to cherry-pick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks really good! It is indeed a way better implementation than the previous one, thank you!
I just tested it and it works like a charm. No modification on my DAG was needed. |
❤️ ❤️ ❤️ ❤️ ❤️ |
Yeah, 3.0.1 is fine imo |
Yeah since it is fully backwards compat it can go in 1.0.1 imo |
Agreed. After @vincbeck comment I am also for 1.0.1 on this one too. And I also added a few more |
The special tests behave strangely. I added a bit better diagnostics as well to see what's going on - now we will have more detailed logging showing what providers are registred and whether any providers are registered when we cannot find the right provider. Let's see what the tests will show. |
788ee64
to
2a42995
Compare
Ough. Diagnostics help. I think we have some side-effects where provider's Manager is cleaned and not restored in some other tests I will look for this - and add protection. |
43c6bbc
to
fcaed66
Compare
Ah... I see now - > also we have to exclude that test for latest botocore, because it requires aiobotocore 🤦 |
0883b51
to
f6ece62
Compare
The common.messaging abstraction should discover common messaging queue providers using the same mechanism as we have for other core extensions. Previously common.messaging had the optional (but not really) dependencies to other providers, but that was not needed and introduced unnecessary coupling. By switching to our built-in discovery mechanism we get immediately all the niceties of provider discovery mechanisms: * queue is provided by the actual provider where the service or integration already is implemented (sqs -> amazon provider, in the future kafka -> kafka provider) * queues are discovered from installed providers * there is no coupling or imports between common.messaging and the providers that implement messaging, the dependency is in the other way - providers that implement messaging depend on common.messaging * airflow providers queues CLI and providers core extensions documentation is automatically generated
f6ece62
to
18b8ed4
Compare
Backport failed to create: v3-0-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker 4f962f4 v3-0-test This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue |
The common.messaging abstraction should discover common messaging queue providers using the same mechanism as we have for other core extensions. Previously common.messaging had the optional (but not really) dependencies to other providers, but that was not needed and introduced unnecessary coupling. By switching to our built-in discovery mechanism we get immediately all the niceties of provider discovery mechanisms: * queue is provided by the actual provider where the service or integration already is implemented (sqs -> amazon provider, in the future kafka -> kafka provider) * queues are discovered from installed providers * there is no coupling or imports between common.messaging and the providers that implement messaging, the dependency is in the other way - providers that implement messaging depend on common.messaging * airflow providers queues CLI and providers core extensions documentation is automatically generated (cherry picked from commit 4f962f4) Co-authored-by: Jarek Potiuk <[email protected]>
The common.messaging abstraction should discover common messaging queue providers using the same mechanism as we have for other core extensions. Previously common.messaging had the optional (but not really) dependencies to other providers, but that was not needed and introduced unnecessary coupling. By switching to our built-in discovery mechanism we get immediately all the niceties of provider discovery mechanisms: * queue is provided by the actual provider where the service or integration already is implemented (sqs -> amazon provider, in the future kafka -> kafka provider) * queues are discovered from installed providers * there is no coupling or imports between common.messaging and the providers that implement messaging, the dependency is in the other way - providers that implement messaging depend on common.messaging * airflow providers queues CLI and providers core extensions documentation is automatically generated (cherry picked from commit 4f962f4) Co-authored-by: Jarek Potiuk <[email protected]>
The common.messaging abstraction should discover common messaging queue providers using the same mechanism as we have for other core extensions. Previously common.messaging had the optional (but not really) dependencies to other providers, but that was not needed and introduced unnecessary coupling. By switching to our built-in discovery mechanism we get immediately all the niceties of provider discovery mechanisms: * queue is provided by the actual provider where the service or integration already is implemented (sqs -> amazon provider, in the future kafka -> kafka provider) * queues are discovered from installed providers * there is no coupling or imports between common.messaging and the providers that implement messaging, the dependency is in the other way - providers that implement messaging depend on common.messaging * airflow providers queues CLI and providers core extensions documentation is automatically generated (cherry picked from commit 4f962f4) Co-authored-by: Jarek Potiuk <[email protected]>
The common.messaging abstraction should discover common messaging queue providers using the same mechanism as we have for other core extensions. Previously common.messaging had the optional (but not really) dependencies to other providers, but that was not needed and introduced unnecessary coupling. By switching to our built-in discovery mechanism we get immediately all the niceties of provider discovery mechanisms: * queue is provided by the actual provider where the service or integration already is implemented (sqs -> amazon provider, in the future kafka -> kafka provider) * queues are discovered from installed providers * there is no coupling or imports between common.messaging and the providers that implement messaging, the dependency is in the other way - providers that implement messaging depend on common.messaging * airflow providers queues CLI and providers core extensions documentation is automatically generated (cherry picked from commit 4f962f4) Co-authored-by: Jarek Potiuk <[email protected]>
The common.messaging abstraction should discover common messaging queue providers using the same mechanism as we have for other core extensions. Previously common.messaging had the optional (but not really) dependencies to other providers, but that was not needed and introduced unnecessary coupling.
By switching to our built-in discovery mechanism we get immediately all the niceties of provider discovery mechanisms:
^ 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 airflow-core/newsfragments.