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

Extension for redis cluster mode #173

Merged
merged 11 commits into from Jul 18, 2020
Merged

Extension for redis cluster mode #173

merged 11 commits into from Jul 18, 2020

Conversation

AIweimd
Copy link
Contributor

@AIweimd AIweimd commented Jun 2, 2020

import redis-py-cluster==2.0.0
CACHE_TYPE=rediscluster
Add cluster parameter of string type:
e.g. host1:port1,host2:port2,host3:port3

@sh4nks
Copy link
Collaborator

sh4nks commented Jun 2, 2020

Hi,
thanks for your PR - however I don't want to add any more backends if there is not more demand for one because I am the one who has to maintain all those.

I'll leave this PR open for now and if there is more demand I might merge it.

@sh4nks sh4nks added the backend label Jun 2, 2020
@DustinMoriarty
Copy link

DustinMoriarty commented Jun 17, 2020

@sh4nks : I support merging this PR. I just created my own backend for cluster mode to use at my company with Elasticache. I was considering submitting a similar PR.

However, I understand that supporting lots of backends can increase maintenance and testing complexity. In that case, would you prefer that each additional backend be a separate package with more of a plugin/extension architecture emphasis
or would it be better for publicly used backends to be maintained within this package?

@gergelypolonkai
Copy link
Collaborator

gergelypolonkai commented Jun 18, 2020

Adding more backends is fun and all, but most authors who do so will abandon their addition as long as it’s merged and the maintainers will have to, well, maintain it.

Since this package already supports using external backend classes, i’d probably go on the plugin/separate package way. It’s the best for everyone.

About this PR, it’s not really a new backend, but the extension of an existing one. I’m with it, too. Maybe it could be incorporated into the plain redis backend?

@sh4nks
Copy link
Collaborator

sh4nks commented Jun 22, 2020

I am thinking about having a section with 'user contributed' backends where we do not offer 'official' support and a section with core backends that are supported by us might be sufficient. Especially if the clients/backends are getting more complex like the RedisSentinel one where testing the client took some more time since I had to setup the redis sentinels.

This one looks kinda simple so I guess we can include it :-)

Copy link
Collaborator

@sh4nks sh4nks left a comment

Choose a reason for hiding this comment

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

Please add some documentation about the CACHE_REDIS_CLUSTER setting here: https://github.com/sh4nks/flask-caching/blob/master/docs/index.rst#configuring-flask-caching

and about the rediscluster cache after the redissentinel cache here:
https://github.com/sh4nks/flask-caching/blob/master/docs/index.rst#redissentinelcache

flask_caching/backends/rediscache.py Outdated Show resolved Hide resolved
@sh4nks
Copy link
Collaborator

sh4nks commented Jul 16, 2020

@AIweimd if you could add this I'd merge the PR.

Please add some documentation about the CACHE_REDIS_CLUSTER setting here: https://github.com/sh4nks/flask-caching/blob/master/docs/index.rst#configuring-flask-caching

and about the rediscluster cache after the redissentinel cache here:
https://github.com/sh4nks/flask-caching/blob/master/docs/index.rst#redissentinelcache

Extension for redis cluster mode
Extension for redis cluster mode
@AIweimd AIweimd closed this Jul 16, 2020
@AIweimd
Copy link
Contributor Author

AIweimd commented Jul 16, 2020

The update document is complete.
It is an honor to contribute to this project.

@AIweimd AIweimd reopened this Jul 16, 2020
@coveralls
Copy link

coveralls commented Jul 16, 2020

Coverage Status

Coverage decreased (-1.3%) to 78.316% when pulling 5659d6e on AIweimd:master into 062401f on sh4nks:master.

Copy link
Collaborator

@sh4nks sh4nks left a comment

Choose a reason for hiding this comment

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

@AIweimd
Thanks for adding the documentation but unfortunately you deleted some existing documentation in your latest commit. Please restore it again :-)

docs/index.rst Outdated Show resolved Hide resolved
Add the documentation of Redis cluster mode.
@sh4nks sh4nks merged commit 3946c72 into pallets-eco:master Jul 18, 2020
@DustinMoriarty
Copy link

@sh4nks: thanks for merging! @AIweimd : thanks for developing! I look forward to testing this out and getting it deployed!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants