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

Redis cluster refactoring #178

Merged
merged 15 commits into from
Nov 5, 2019
Merged

Conversation

hofrob
Copy link
Contributor

@hofrob hofrob commented Dec 17, 2018

Q A
Is bugfix? yes
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #147 #143 #142 #136 #135 #134 #66

@hofrob
Copy link
Contributor Author

hofrob commented Dec 17, 2018

Connections to redis are now pooled, to enable redirects without opening and closing connections during a request.

If redis is running in cluster mode, it is possible to supply a hashtag for MGET and MSET operations. If there are now hashtags, it falls back to GET and SET.

To test this locally I used a ready-to-use docker cluster container:

docker run --network=some_local_network --network-alias=redis-cluster grokzen/redis-cluster:redis_version

Put a PHP container with Yii in the same network (some_local_network), select a redis version (redis_version), and try the example.

@hofrob
Copy link
Contributor Author

hofrob commented Dec 17, 2018

I should implement some tests for this change. Probably by using the aforementioned docker container. But to be honest, I have no idea where to start. If someone could point me in the right direction, I'd be happy to get started.

@hofrob
Copy link
Contributor Author

hofrob commented Dec 18, 2018

@cebe

This PR would fix the problem you mentioned here: #147 (comment)

@cebe
Copy link
Member

cebe commented Feb 1, 2019

Thanks for your work on this. I am planning to use a redis cluster in the near future and will review this PR when I get to that.

@hofrob
Copy link
Contributor Author

hofrob commented Feb 2, 2019

I can give you an update from what we're using now. Turns out this solution works, but somehow it was the slowest of all the solutions we tried (our production env runs on AWS EB with extensive use of redis ElastiCache). Our requests were just about 10-15% slower than before. But it was noticeable.

And yes, weirdly enough opening and closing the connections after redirects was faster. But I did not do any more research on that because....

We then tried the redis pecl extension. It supports cluster mode completely and we're now at about the same request duration we were before.

To be able to use the pecl extension with \yii\caching\Cache I just had to write a very simple wrapper (about 80 lines). The only thing left for us to completely switch to this extension is rewriting the yii2-queue. It's unfortunate that the method names are written differently.

  • hdel vs hDel
  • lpush vs lPush
  • zadd vs zAdd

So I've not yet tackled this. I think it would be great if you could switch from yii2-redis to the pecl extension without having to change something in yii2-queue. If you or the yii devs in general are interested in implementing this, maybe I can help.

@samdark
Copy link
Member

samdark commented Apr 16, 2019

That is interesting, especially for 3.0.

@samdark
Copy link
Member

samdark commented Aug 27, 2019

@cebe did you have a chance to use Redis cluster?

@samdark samdark added the status:code review The pull request needs review. label Aug 27, 2019
@samdark samdark requested a review from a team August 27, 2019 13:09
@pacifier17
Copy link

@samdark do you think we can get this merged soon? We are looking to use cluster Redis for our use case. We did test out the fork and it works with cluster redis config.

@samdark samdark added this to the 2.0.11 milestone Nov 4, 2019
src/Cache.php Outdated Show resolved Hide resolved
src/Cache.php Outdated Show resolved Hide resolved
src/Cache.php Outdated Show resolved Hide resolved
src/Cache.php Outdated Show resolved Hide resolved
src/Cache.php Outdated Show resolved Hide resolved
src/Connection.php Show resolved Hide resolved
src/Cache.php Outdated Show resolved Hide resolved
src/Cache.php Show resolved Hide resolved
src/Connection.php Show resolved Hide resolved
src/Connection.php Show resolved Hide resolved
@samdark
Copy link
Member

samdark commented Nov 4, 2019

Overall it looks OK. Some minor adjustments and changelog line are needed.

@samdark
Copy link
Member

samdark commented Nov 4, 2019

Unit tests would be awesome as well.

@samdark
Copy link
Member

samdark commented Nov 4, 2019

@hofrob would you please apply suggestions?

@hofrob
Copy link
Contributor Author

hofrob commented Nov 4, 2019

changelog line are needed.

Since this is a really old branch, is there a preferred way how to update it? I'd just rebase it (and then add the changelog).

@samdark
Copy link
Member

samdark commented Nov 4, 2019

Either merging master into it or rebasing.

@hofrob
Copy link
Contributor Author

hofrob commented Nov 5, 2019

Unit tests would be awesome as well.

After a quick look at the sources I'm not really sure how to configure/start/use a redis cluster for tests.

@samdark samdark merged commit 447141e into yiisoft:master Nov 5, 2019
@samdark
Copy link
Member

samdark commented Nov 5, 2019

Merged. Thank you very much!

@samdark samdark added type:enhancement Enhancement and removed status:code review The pull request needs review. labels Nov 5, 2019
@pacifier17
Copy link

Thanks a lot @samdark and @hofrob for merging these changes in so quickly. I really appreciate it.

@samdark
Copy link
Member

samdark commented Nov 5, 2019

Not so quickly. Look how much time it took me to get to it and review/merge it :) @hofrob sorry for that btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants