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

Replace aioredis with redis-py #151

Merged
merged 1 commit into from
Sep 4, 2022
Merged

Replace aioredis with redis-py #151

merged 1 commit into from
Sep 4, 2022

Conversation

Xiretza
Copy link
Contributor

@Xiretza Xiretza commented Aug 22, 2022

aioredis has been merged into redis-py and will no longer be maintained as a separate project.

Fixes #150.

@Xiretza
Copy link
Contributor Author

Xiretza commented Aug 22, 2022

Unfortunately I don't have a redis setup to actually test this thoroughly, so it'd be great if someone else could give it a go.

@tasn
Copy link
Member

tasn commented Aug 22, 2022

Oh, good to know! Let me know once this has been properly tested. I also don't have time for at least a week. :(

@jman-schief
Copy link

jman-schief commented Aug 24, 2022

Hi, I came here from #132, I had issues running etesync with a Redis endpoint configured. I have not yet the picture completely clear but I think @tasn is right that the Redis connection was not deeply tested. etesync crashes with a Redis endpoint configured.

@Xiretza I've tried your patch and dug a little bit further. I think we cannot yet use the old aioredis connection pool APIs for the time being (see redis-py async client documentation), the merge of aioredis into redis-py left some bits behind.

I've tried quickly hacking your patch as follows:

--- a/etebase_server/fastapi/redis.py
+++ b/etebase_server/fastapi/redis.py
@@ -12,12 +12,11 @@ class RedisWrapper:
 
     async def setup(self):
         if self.redis_uri is not None:
-            self.redis = await aioredis.create_redis_pool(self.redis_uri)
+            self.redis = await aioredis.from_url(self.redis_uri)
 
     async def close(self):
         if hasattr(self, "redis"):
-            self.redis.close()
-            await self.redis.wait_closed()
+            await self.redis.close()

and /at least/ I don't see crashes on startup/shutdown of etesync.

What do you think? makes sense?

That's all I can contribute for the time being, hope it helps! :)

@tasn
Copy link
Member

tasn commented Aug 24, 2022

It's not the end of the world not to use a pool, but it's much much better.

@tasn
Copy link
Member

tasn commented Aug 24, 2022

Actually, I take it back. I think with pub/sub, which we use, it's probably a big deal.

@jman-schief
Copy link

@tasn to further clarify my previous comment, I just meant that the old aioredis connection pool API is not available and one should probably switch to another one (as per the documentation link).

@tasn
Copy link
Member

tasn commented Aug 25, 2022

Yeah, I understood that @jman-schief, though thanks for the clarification. I just re-emphasized, that I think a pool is needed, or at least the code needs to be changed to reconnect every time (less than ideal).

@tasn
Copy link
Member

tasn commented Sep 4, 2022

OK, it looks like redis-py does indeed support pooling, we just need to slightly change how we create the object (as discussed above).

@Xiretza, got time to take a look, or should I see if I can adjust this PR?

@tasn
Copy link
Member

tasn commented Sep 4, 2022

I pushed the adjustments to Xiretza-redis-py on this repo. Can you pull the changes and add to this PR? I'll happily merge it after.

aioredis has been merged into redis-py and will no longer be maintained
as a separate project.
@tasn tasn merged commit c61dd86 into etesync:master Sep 4, 2022
@victor-rds
Copy link
Contributor

@tasn could you make a new release?

@tasn
Copy link
Member

tasn commented Oct 4, 2022

Done! Thanks @victor-rds for the reminder. In my head you're still tracking master. :)

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

Successfully merging this pull request may close these issues.

Gentoo: aioredis to redis-py
4 participants