-
Notifications
You must be signed in to change notification settings - Fork 16.4k
AIP-82: Add RedisPubSubMessageQueueProvider #53556
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
Conversation
00cf5d0 to
5ab9f82
Compare
|
Question. Would it be possible to add an integration test for that one? We already have integration tests with redis run in our CI and for me it would be cool if we can run the tests of message queue with "real" readis rather than based on unit tests. Some pointers for you: |
|
Thanks for pointing out! I'll give it a try. |
vincbeck
left a comment
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.
One nit but overall it looks good! Very nice to see other providers onboarding 🚀 I agree with Jarek's comment too, if we can have integration tests, we should do it :)
5ab9f82 to
783d5e8
Compare
|
Hi @jason810496, during the period of adding integration test, I found that a bug exists in my unit test. I could address the issue if my understanding is correct. However, I would appreciate hearing your suggestions first. Thank you! |
2cca9cb to
71328d7
Compare
You're entirely correct! Thanks for the catch! I created #54135 to fix it in SQS and Kafka |
jason810496
left a comment
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.
Thanks for the update.
I just noticed an issue regarding URI usage, and we might need to have further discussion on this.
* AIP-82: Add RedisMessageQueueProvider * change docs title level * specify pubsub in naming * fix import bug * add integration test * fix queue path in system test * set minimum version for common.messaging
Why
Related issue: #52712.
Related PR: #52917.
What
Introduce support for Redis as a
MessageQueueProvider, extending Event-Driven Scheduling.^ 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.