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

Async RedisCluster missing pubsub() support #2497

Closed
itsBrady opened this issue Dec 9, 2022 · 4 comments
Closed

Async RedisCluster missing pubsub() support #2497

itsBrady opened this issue Dec 9, 2022 · 4 comments

Comments

@itsBrady
Copy link

itsBrady commented Dec 9, 2022

Version: 4.4.0

Platform: python 3.8,

Description: async RedisCluster class does not have pubsub() function, or any pubsub commands.

Looking to move from aioredis to this library for cluster mode support. Making a simple test case I get this error:

Traceback (most recent call last):
|   File "src/test.py", line 26, in <module>
|     loop.run_until_complete(main())
|   File "/usr/local/lib/python3.8/asyncio/base_events.py", line 616, in run_until_complete
|     return future.result()
|   File "src/test.py", line 21, in main
|     test = cluster.pubsub()
| AttributeError: 'RedisCluster' object has no attribute 'pubsub'

Looking in the documentation, I see that the sync client of RedisCluster has pubsub included but not the async RedisCluster. Is there a reason for this? Any workarounds that can be shared, or is the expectation to use execute_command to manually handle these commands?

Example code:

import asyncio
import redis.asyncio as redis

cluster = redis.RedisCluster(
    host="", password="", read_from_replicas=True
)

async def main():
    await cluster.initialize()
    test = cluster.pubsub()
    await cluster.close()


loop = asyncio.get_event_loop()
loop.run_until_complete(main())
loop.close()
@itsBrady
Copy link
Author

@utkarshgupta137 if you have time to chat I'd love to help add this functionality

My current sticking point is that the ClusterPubSub class for the sync implementation seems to work with ConnectionPool instances, while the async RedisCluster definition each cluster node seems to manage their own list of free/available Connections.

Curious if I should try to port the async stuff to more closely match the sync stuff, or copy and paste the pubsub stuff and change it to be okay with just Connections instead of ConnectionPools

@utkarshgupta137
Copy link
Contributor

@itsBrady Sorry for replying late.

I had written the async impl with performance in mind. Hence I opted to manage connections directly instead of creating an async client & then getting a ConnectionPool from there. This led to some significant differences in sync & async internals (as well as standalone/cluster internals), but it led to about 10-15% improvement in performance AFAIR.
The use of a deque instead of a list for storing free connections & the lack of support of forking (removing pid checks & required locks) made a significant difference in performance.
Hence, I would not recommend simply using async ConnectionPools from the standalone client. Ideally, the same changes should be ported to the async ConnectionPool & then we could use the connection pool for the cluster client.

I've never used PubSub & I'm unfamiliar with the code, so I'm not sure why you need ConnectionPools specifically for using PubSub. You should probably look at the differences between standalone & cluster sync PubSub & then apply the same to the async PubSub, instead of porting the sync cluster to async. (i.e. do async standalone -> async cluster, not sync cluster -> async cluster). Also, the coredis library already has support for PubSub for async cluster. I think it has a significantly different architecture to the redis-py library, but it may helpful.

I've switched jobs & no longer use Redis (or even Python), so I won't be able to bring feature parity to async cluster client as I hoped to do. But if you've any questions, feel free to ping or mention me.

@itsBrady
Copy link
Author

@utkarshgupta137 No problem at all!

coredis seems like a good option, thank you for sharing I did not know it existed, I will continue to eval and let you know if I have any more questions. Thanks for your time friend!

@chayim chayim changed the title async RedisCluster missing pubsub() support Async RedisCluster missing pubsub() support Jan 18, 2023
@uglide
Copy link
Contributor

uglide commented Jun 15, 2023

Duplicate of #2219

@uglide uglide marked this as a duplicate of #2219 Jun 15, 2023
@uglide uglide closed this as completed Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants