add redis sentinel support

[L1ghtman2k]

I know that there were multiple requests, and most of them were denied since the primary author was not working with sentinel. Now that this project got passed to JazzBand, would it be possible to request sentinel support for this project?

[craigargh]

I need this as well. I'm going to try and implement it as a new Redis client class for the project that I'm working on. I'll raise it as a pull request if I get it to work

[craigargh]

There's also old pull requests that added Sentinel support https://github.com/niwinz/django-redis/pull/125/files

Also looks like it was included in the project, but was later removed for some reason https://github.com/niwinz/django-redis/pull/51/files

[L1ghtman2k]

There's also old pull requests that added Sentinel support https://github.com/niwinz/django-redis/pull/125/files

Also looks like it was included in the project, but was later removed for some reason https://github.com/niwinz/django-redis/pull/51/files

I believe it was removed because primary maintainer had no use for it. But now that the project moved to JazzBand there is a potential to support this feature.

[craigargh]

If you're in a hurry to get this working the code in this repo seems to do the job: https://github.com/SpatialBuzz/django-redis-sentinel

You just need to add the changes in this pull request: https://github.com/SpatialBuzz/django-redis-sentinel/pull/7

I set it up yesterday and it looks like it is working fine. The biggest hurdle was understanding the correct format for the location setting.

[craigargh]

From the project's history, it looks like the django-redis-sentinel code was originally part of the codebase, but was removed. When someone made a pull request to add it back in, the maintainer asked for it to be a separate repo, which is why the django-redis-sentinel repo exists.

If I (or someone else) wants to make a pull request for a sentinel client, it could probably just be based off the django-redis-sentinel code.

[terencehonles]

If I (or someone else) wants to make a pull request for a sentinel client, it could probably just be based off the django-redis-sentinel code.

460 considered all the links you provided @craigargh (thanks!), but I took it a slightly different direction. I've written the tests, and I'll likely be looking to test this in a more production like scenario in the near future (I tested with a small replica set + sentinels locally)

[terencehonles]

Just to follow up, since it was asked in https://github.com/jazzband/django-redis/pull/460#issuecomment-716109531, we've been using #460 in production without any issues. I'd move that we should consider reviewing #460 and seeing if there are any issues with merging and supporting Redis sentinels in the way I proposed.

[severindellsperger]

Any updates here? I took a look at the code of the PR, and it seems fine. I like to use it in production, and I saw that the PR is still not merged.

@WisdomPill - I really am looking forward to using this feature. I also checked the PR conversation - is there any plausible reason not to merge it? It seems to me that many people like to use it, especially because the other packages are not maintained... Merging it with a warning would also be fine for me.

[WisdomPill]

I am not against merging it, it is just that I never used sentinel so I am not able to review it, since I do not know if it is okay or not, as a matter of fact there is the help-needed label on the PR and the issue. I see many people are interested, if somebody could step in and review it, he/she is welcome. If nobody can step in then I will merge it, it is just that I would prefer some help if possibile, like a comment on the pr to signal that it is okay

[laurentL]

hi,

I can try to test the merge request on a hudge plateform. Is-it possible to create a branch with this PR merged ?

[WisdomPill]

@laurentL I think you can use the repo of the PR for testing, anyway it has been sometime and I see in the PR that there are no real issue with the help of @severindellsperger, so I would say that if @laurentL can't test it within a week, I can merge and mark the feature as experimental. Do all of you agree? Just put a few thumbs up in this comment so I know that you are following and agree

[laurentL]

my deployment system does not allow me to use PR or tag url to import libraries. I must admit that if you can create an experimental package it saves me a lot of time.

On Sat, Mar 20, 2021 at 5:44 PM Anas @.***> wrote:

@laurentL https://github.com/laurentL I think you can use the repo of the PR for testing, anyway it has been sometime and I see in the PR that there are no real issue with the help of @severindellsperger https://github.com/severindellsperger, so I would say that if @laurentL https://github.com/laurentL can't test it within a week, I can merge and mark the feature as experimental. Do all of you agree? Just put a few thumbs up in this comment so I know that you are following and agree

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jazzband/django-redis/issues/426#issuecomment-803402102, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6SCOZL4PVSKHCGGZEPFITTETGIHANCNFSM4J56LCFA .

[laurentL]

has the experimental package been created?

[severindellsperger]

@WisdomPill I implemented the changes yesterday in our dev environment. It works without any problems - from my side, the PR can be merged.

For all others who are interested in the settings I used:

# Settings for Redis Cache 
DJANGO_REDIS_CONNECTION_FACTORY = "django_redis.pool.SentinelConnectionFactory"
SENTINELS = [(REDIS_SENTINEL_SERVICE_NAME, REDIS_SENTINEL_PORT)]
CACHES = {
    "default": {
        "BACKEND": "django_redis.cache.RedisCache",
        "LOCATION": f"redis://:{REDIS_PASSWORD}@{REDIS_MASTER}?db={REDIS_GRAPH_CACHE_DB}",
        "OPTIONS": {
            "CLIENT_CLASS": "django_redis.client.SentinelClient",
            "SENTINELS": SENTINELS,
        },
    }
}

PS: REDIS_MASTER is the connection string of the actual Redis nodes/service.

[WisdomPill]

good to know, I am raging it right now, thanks @severindellsperger.

Also, I am sorry, @laurentL I saw your message and thought about writing how to create a wheel package of the library but ended up forgetting about it.

[severindellsperger]

@WisdomPill, thanks for merging it into main. Is it possible for you to create a new PyPI package with the changes. Unfortunately, most users are not permitted to install it via pip via git tag/info in production.

[WisdomPill]

I am afraid not, I do not have the permission to make new releases, I need to ask

[severindellsperger]

It would be handy if you could contact the maintainers, who have permission to do it.

[terencehonles]

@WisdomPill if acceptable by the parties that have push access to PyPI I could probably write a github action which packages the wheel and uploads it to PyPI on tag pushes (or some other criteria). I would just need to confirm what scripts/steps are run to package the wheel (in case there is something special) and then someone would need to add the PyPI credentials to the repository secrets stored by GitHub so that said workflow could actually complete the push.

[WisdomPill]

maybe you could ask in #430

[terencehonles]

It looks like I misunderstood how jazzband projects are released. I don't think this is needed or it's something that I would need to look into more. It looks like there are other steps needed in #430 regardless.