Document a method for sharing a connection pool with the rest of the application

[jazzband]

Document a method for sharing a connection pool with the rest of the application

[niwinz]

Hi! Seems very good idea include a simple way to set custom instance of redis connection pool. But currently the code is no ready to do it.

Do you have any good idea of how I should implement it?

[wolever]

Okay… after some poking around, I'd propose replacing pool.py with something like this:

# -*- coding: utf-8 -*-

from django.conf import settings as django_settings
from django.core.exceptions import ImproperlyConfigured
from redis import Redis
from redis.connection import (
    ConnectionPool as RedisConnectionPool,
    Connection, DefaultParser, UnixDomainSocketConnection,
)

from .util import load_class


__all__ = [
    'get_connection',
    'get_connection_from_pool',
    'get_or_create_connection_pool',
]

class DjangoRedisConnectionPool(object):
    def __init__(self, settings=None):
        self._pools = None
        self._settings = settings or django_settings

    def _load_cls(self, cls, default):
        try:
            path = getattr(self._settings, cls)
        except AttributeError:
            return default
        return load_class(path)

    def parse_connection_string(self, constring):
        """
        Method that parse a connection string.
        """
        try:
            host, port, db = constring.split(":")
            port = port if host == "unix" else int(port)
            db = int(db)
            return host, port, db
        except (ValueError, TypeError):
            raise ImproperlyConfigured("Incorrect format '%s'" % (constring, ))

    def get_connection(self, cxn_string):
        factory = self._load_cls("DJANGO_REDIS_CONNECTION_FACTORY",
                                 self.get_connection_from_pool)
        return factory(cxn_string)

    def get_connection_kwargs(self, cxn_string):
        host, port, db = self.parse_connection_string(cxn_string)
        kwargs = {
            "db": db,
            "parser_class": self._load_cls("PARSER_CLASS", DefaultParser),
            "password": self._settings.get('PASSWORD', None),
        }

        if host == "unix":
            kwargs.update({'path': port, 'connection_class': UnixDomainSocketConnection})
        else:
            kwargs.update({'host': host, 'port': port, 'connection_class': Connection})

        if 'SOCKET_TIMEOUT' in self._settings:
            kwargs.update({'socket_timeout': int(self._settings['SOCKET_TIMEOUT'])})

        pool_kwargs = self._settings.get("CONNECTION_POOL_KWARGS", {})
        kwargs.update(pool_kwargs)
        return kwargs

    def get_connection_from_pool(self, cxn_string):
        """
        Creates a redis connection with connection pool.
        """
        kwargs = self.get_connection_kwargs(cxn_string)
        pool_cls = self._load_cls("CONNECTION_POOL_CLASS", RedisConnectionPool)
        connection_pool = self.get_or_create_connection_pool(pool_cls, **kwargs)
        connection = Redis(connection_pool=connection_pool)
        return connection

    def get_or_create_connection_pool(self, pool_cls, **params):
        key = str(params)
        if key not in self._pools:
            self._pools[key] = pool_cls(**params)
        return self._pools[key]


default_pool = DjangoRedisConnectionPool()
get_or_create_connection_pool = default_pool.get_or_create_connection_pool
get_connection = default_pool.get_connection
get_connection_from_pool = default_pool.get_connection_from_pool

Some fiddling would need to be done to maintain complete backwards compatibility (ex, exactly how it would be called from the DefaultClient)… but that's the general idea I'd propose.

What do you think? Worth pursuing?

[niwinz]

Hmm I like the approach! Also it split the connection parameter parsing out of clients.

Thanks for your time!

[niwinz]

Hi!

Now I have initial implementation for plugable connection factory: d84b8dc

I take some ideas from your suggested example, but with slightly distinct approach.
Now, if you want change the default behavior, you should extend a ConnectionFactory class and overwrite the proper method.

Do you can give some feedback? Thanks!

[wolever]

Awesome, this looks great. Yea, if you don't mind breaking backwards compatibility, your implementation is much cleaner. I'll leave a couple minor notes on the diff, which you can feel free to implement or disregard.

[niwinz]

Thanks for your suggestions!

About backward compatibility, a public api is completely backward compatible and I don't have much problems to change internal clients api, in this case the change is small but with much better implementation ;)

Now, I have pushed some fixes with additional compatibility fixes for other clients. Can you test it on your environments and report any issue. If every thing works fine. I release a new version in few days.

Again thanks for you time.

[miohtama]

I was looking up how to set up Redis connection pool with Django and this popped up. Just wanted to say you guys rock! I can help with the testing.

[miohtama]

Just wishing that can you updare README for those who are not yet that well-versed setting up Redis and django-redis? E.g. what are the requirements of setting up connection pool (do I need to change Redis setting too from the defaults...) and corresponding django-redis OPTIONS.

[niwinz]

Year you are right, the documentation about this lacks! I go to add it ;)

[niwinz]

Now added documentation for it on the branch (with some other improvements): https://github.com/niwibe/django-redis/compare/plugable-connection-factory, on it merged, the docummentation will to be available on the githubpages ;)

Any test is welcome before merge it to master and make a new release ;)

[miohtama]

Is there a way to check if my Redis connection is pooled or not? E.g. from Django shell somehow get a low level connection information and check if it has any kind pooled flag or recycled connection id set?

[niwinz]

Thanks everybody! Now is merged #67