martinrusev / django-redis-sessions

Session backend for Django that stores sessions in a Redis database
BSD 3-Clause "New" or "Revised" License
495 stars 106 forks source link

Allow configuration of connection pool to use #26

Closed jeffsmohan closed 10 years ago

jeffsmohan commented 10 years ago

Every Redis instance, by default, creates and manages its own connection pool (https://github.com/andymccurdy/redis-py#connection-pools). In certain situations, it's useful to be able to manage connection pooling yourself. (In my case, I have a limited number of total connections available through a third-party hosted Redis service.)

I would be happy to take a stab at coding this up and sending a pull request, if you'd be open to it. I imagine it would be an optional setting such as:

SESSION_REDIS_CONNECTION_POOL = 'myproj.utils.RedisConnectionPool'

Do you have any initial guidance on how you'd want such a feature implemented? Would you be open to this?

hellysmile commented 10 years ago

Previous releases used 1 connection for each request, now 1 running django instance have 1 connection to redis server, so take a look on your django processes\threads\etc

jeffsmohan commented 10 years ago

Understood. However, I also use Redis for other purposes throughout my code, and it would be helpful to be able to share a connection pool between all of them. As it is, each Django instance requires at least 2 connections, since I have no control over the connection django-redis-sessions is using.

hellysmile commented 10 years ago

you can just

from redis_sessions.session import redis_serever

redis_server.do_something()
jeffsmohan commented 10 years ago

That's really not a good solution, either.

First, the location and naming of the redis_server in this package is not documented and is not a feature, so there's no guarantee it won't be moved or renamed or changed in a future release. I don't want to be in a position where updating my version of django-redis-sessions breaks all my internal Redis code.

Second, if I'm using more than one library that suggests this solution, I wouldn't be able to follow the advice. I'd be stuck with at least two connection pools again. In order for django-redis-sessions to play nice with other packages, it ought to make the connection configurable.

What is your resistance to adding this functionality?

hellysmile commented 10 years ago

I am not maintainer at all, just have a pair of commits to this library

jeffsmohan commented 10 years ago

Gotcha! Appreciate the insights. Would love to hear from the maintainer on how he/she feels about this.

martinrusev commented 10 years ago

@jeffschenck Connection pool support makes a lot of sense. If you can get started with the initial version, I will merge in the main repository

jeffsmohan commented 10 years ago

Sent pull request #27 for this, so I figure we can continue any discussion over there. Thanks!