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

migrate aioredis to v2.0 #285

Closed
vvxhid opened this issue Dec 14, 2021 · 10 comments
Closed

migrate aioredis to v2.0 #285

vvxhid opened this issue Dec 14, 2021 · 10 comments

Comments

@vvxhid
Copy link

vvxhid commented Dec 14, 2021

Connecting to redis is no longer done by calling await aioredis.create_pool(...).
link

@dimonji
Copy link

dimonji commented Dec 20, 2021

Plus 1, any plans for upgrading airedis?

@LincolnPuzey
Copy link

This has been discussed in #268 and mentioned here.

Upgrading to 2.0 is the plan, just no one has done the work yet.

@ipmb
Copy link
Contributor

ipmb commented Mar 3, 2022

This library is now part of the mainstream Redis library as of 4.2.0-rc1 redis/redis-py#1899

@carltongibson
Copy link
Member

Oh, that is good!

Very happy to take a PR implementing this swap over.

@carltongibson carltongibson pinned this issue Mar 3, 2022
@ipmb
Copy link
Contributor

ipmb commented Mar 3, 2022

I dug into this a bit, could you explain what the reasoning is for this?

async def create_conn(self, loop):
# One connection per pool since we are emulating a single connection
kwargs = {"minsize": 1, "maxsize": 1, **self.host}
if not (sys.version_info >= (3, 8, 0) and AIOREDIS_VERSION >= (1, 3, 1)):
kwargs["loop"] = loop
if self.master_name is None:
return await aioredis.create_redis_pool(**kwargs)

It's creating a single connection pool then treating and referring to it as a connection. Some of the paradigms the library uses like checking if the pool is closed don't map in redis-py. You can check if a connection is closed and you can release all the connections in a pool, but you don't check if a pool is closed.

@carltongibson
Copy link
Member

It was added for the sentinel support. 🤔 @qeternity — Can you comment on the reasoning there? Thanks.

@qeternity
Copy link
Contributor

qeternity commented Mar 4, 2022

Hi @ipmb - The API for pools and connections is very similar (at least in aioredis 1.3.x). We can't use a raw redis connection with sentinel because a sentinel connection is actually two redis connections: one pubsub connection to a sentinel, and then a connection to the actual redis instance. You need to listen to the pubsub connection for changes from the sentinel(s). This logic was all orchestrated automatically by the aioredis sentinel pool. However, because non-pubsub layer does not use the library pooling, and marshals connections manually, we don't want to risk letting the sentinel connections in the library pool grow, because we will end up having many pools. All of this was done to make sentinel drop-in compatible with the non-pubsub layer.

I have not looked at the new redis-py migration, but in the 1.3.x branch, you absolutely could check if a pool is closed: https://github.com/aio-libs/aioredis-py/blob/8a207609b7f8a33e74c7c8130d97186e78cc0052/aioredis/sentinel/pool.py#L160

@ipmb
Copy link
Contributor

ipmb commented Mar 4, 2022

Yeah, it's not available here https://github.com/redis/redis-py/blob/c5d19b8571d2b15a29637f56a51b0da560072945/redis/asyncio/connection.py#L1379-L1538

I'm guessing we can iterate over the connections ourselves and verify they have all been closed.

@qeternity
Copy link
Contributor

Yep, there were a lot of breaking changes in 2.x which is why we never migrated.

That said, the fix here should be to use pools instead. This library has basically reimplemented its own pool manager.

@carltongibson
Copy link
Member

This was done in #296 — will be in 4.0. 4.0.0b2 is available for testing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants