-
-
Notifications
You must be signed in to change notification settings - Fork 944
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
support redis cluster transport #2204
base: main
Are you sure you want to change the base?
support redis cluster transport #2204
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2204 +/- ##
==========================================
+ Coverage 81.51% 81.80% +0.29%
==========================================
Files 77 78 +1
Lines 9526 9949 +423
Branches 1153 1229 +76
==========================================
+ Hits 7765 8139 +374
- Misses 1569 1597 +28
- Partials 192 213 +21 ☔ View full report in Codecov by Sentry. |
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.
lets see if the CI passes
dca3e75
to
9983557
Compare
f4d275a
to
f5c7356
Compare
5351519
to
6c0cadf
Compare
c87be51
to
6986510
Compare
Only one test is failing, due to connection failure |
…nsport' into feature/support_rediscluster_transport
for more information, see https://pre-commit.ci
…nsport' into feature/support_rediscluster_transport
Check linter error :) |
Sorry for the failure, I will fix it and improve the test coverage. |
I am following it and already had reviewed it twice. Will have an in depth review again tomorrow. No worries. Thanks for picking my work |
Added docs for kombu.transport.rediscluster, now I'm sure there won't be problems anymore |
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.
Finally that everything passes on the CI, we can properly review the code.
That being said, we’re currently in a release phase so we can’t merge it until we complete the release, after the new year holidays.
Good work and thank you for fixing everything.
Would this also support Redis cluster for backend? |
Co-authored-by: bashir-abdelwahed <[email protected]>
|
||
conn_params['connection_pool_class'] = ManagedConnectionPool | ||
|
||
conn_params['url'] = f'redis://{conn_params["host"]}:{conn_params["port"]}' |
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.
@zs-neo thanks for all of your work on this! I'm testing out this PR right now and noticed that TLS support is broken. it looks as though it might be as simple as using the rediss
scheme here with the ssl
logic above?
Attempt to address #1021
Thank you very much for your code, it helps us a lot. @auvipy
We use redis-py instead of redis-py-cluster because redis-py-cluster has been merged into redis-py.
Celery works fine on our cluster with multi producers and multi consumers, when a node goes down, it can automatically switch.