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

Use hiredis #2577

Merged
merged 6 commits into from
May 5, 2020
Merged

Use hiredis #2577

merged 6 commits into from
May 5, 2020

Conversation

Andrew-Chen-Wang
Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang commented Apr 29, 2020

Description

Using hiredis, which is developed by the redis community, too, is probably best performance wise.

Benchmarks (came when I was using aioredis for websocket pubsub): https://github.com/popravich/python-redis-benchmark

Rationale

Since Redis is already used in this project, why not use hiredis? Windows is not supported by Redis and Hiredis; however, Redis could be supported via Docker.

Another thing is most production-ready Django projects don't use Windows servers. So I've put hiredis as a mandatory thing in production (since celery uses redis in local, too, so separate occasions for the hiredis requirements).

Use case(s) / visualization(s)

More performant redis

@jcass77
Copy link
Contributor

jcass77 commented Apr 30, 2020

I was always under the impression that redis-py would revert to using the HiredisParser automatically if it was available?

Are there some specific implementation details in the Django cache that requires this to be done explicitly? (I haven't checked).

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Apr 30, 2020

redis-py would, but I don't think Django-redis does. I've never tested it, but I couldn't find an issue saying Django-redis was default using it.

Edit: I did test it, but it was so varying in my own tests (using debug toolbar), that there wasn't much of a noticeable difference.

@jcass77
Copy link
Contributor

jcass77 commented Apr 30, 2020

Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK to add this. Noted a few edge cases with Docker + Windows.

{{cookiecutter.project_slug}}/requirements/base.txt Outdated Show resolved Hide resolved
{{cookiecutter.project_slug}}/requirements/production.txt Outdated Show resolved Hide resolved
@@ -12,6 +12,9 @@ Collectfast==2.1.0 # https://github.com/antonagestam/collectfast
{%- if cookiecutter.use_sentry == "y" %}
sentry-sdk==0.14.3 # https://github.com/getsentry/sentry-python
{%- endif %}
{%- if cookiecutter.use_docker == "n" and cookiecutter.windows == "y" %}
hiredis==1.0.1 # https://github.com/redis/hiredis-py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused here. The line tells me that if people are not using docker and they have said yes to windows, we force the use of hiredis in production? Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's just wrong to use a Windows machine to deploy a server. If you take a look at base.txt, it has a different condition for if you use Windows or Docker. If you do use Windows but not docker, you cannot build hiredis. But, since you wanted to use Redis, you'll also get hiredis for production, assuming, with hope, that you deploy with a unix.

However, if you use Docker on Windows, you should be able to build hiredis, and thus it can go into base.txt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks - that clarifies a lot. I see this is mentioned in the description as well but it didn't click until now.

Thanks a lot!

@browniebroke browniebroke merged commit 9075bdb into cookiecutter:master May 5, 2020
@browniebroke
Copy link
Member

Thanks 🎉

@Andrew-Chen-Wang Andrew-Chen-Wang deleted the hiredis branch May 5, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants