-
Notifications
You must be signed in to change notification settings - Fork 197
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
Allow passing extra connection kwargs with host address #337
Allow passing extra connection kwargs with host address #337
Conversation
@carltongibson what do you think about this pull request? |
@nasir733 I haven't had a chance to review it yes, otherwise I'd have commented. (@mentioning me doesn't help. I feel we've discussed this before... 😅) If you want to give it a run, verify it works (or not) for you and comment along those lines, then that would be a useful thing to do. (If it does work you're then able to use this branch whilst waiting for a release.) |
Again with test hangs, I helped fight these not too long ago. I think with #342 also fixing pubsub's connection the two can be combined into one. |
The approach in my PR seemed to just replace this issue with another:
Perhaps a parallel with rq/rq#1383 |
@@ -130,7 +130,8 @@ def create_pool(self, index): | |||
host = self.hosts[index] | |||
|
|||
if "address" in host: | |||
return aioredis.ConnectionPool.from_url(host["address"]) | |||
address = host.pop("address") |
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.
For this to work, we shouldn't mutate the original dict. We need to deepcopy(host)
, and mutate that here.
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.
Also see #341 which addresses the same mutation issue for the branch below.
It might make sense to just copy.deepcopy()
on line 130 to avoid the risk of mutation in the downstream redis
library too.
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.
This should also be addressed in the pubsub layer (note that pubsub already performs a copy of host dict in RedisSingleShardConnection
init):
channels_redis/channels_redis/pubsub.py
Line 351 in a7094c5
pool = aioredis.ConnectionPool.from_url(self.host["address"]) |
Resolved in #352. Thanks. |
This should fix some of the issues with #331 by allowing to pass extra connection arguments with the host configuration.
The specific case that this helps me with is passing the ssl kwargs into the connection as advised by Heroku redis docs