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

Implement redis redirects (cluster), switch to enable/disable MGET #147

Closed
wants to merge 3 commits into from

Conversation

hofrob
Copy link
Contributor

@hofrob hofrob commented Apr 19, 2018

multi get in a cluster with read replicas only works when the keys to be
retrieved are allocated in the same hash slot
https://redis.io/topics/cluster-spec#keys-hash-tags

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues comma-separated list of tickets # fixed by the PR, if any

multi get in a cluster with read replicas only works when the keys to be
retrieved are allocated in the same hash slot
https://redis.io/topics/cluster-spec#keys-hash-tags
@hofrob
Copy link
Contributor Author

hofrob commented Apr 19, 2018

When I have time I may take a look at how to solve this with using hash tags in multiSet as proposed in the redis documentation.

Avoiding multiGet would solve this, but you would also need to avoid yii\caching\TagDependency.

separate option to turn off `multiGet` even when no read replicas are
configured
@hofrob
Copy link
Contributor Author

hofrob commented Apr 20, 2018

Turns out, this still doesn't solve the issues we have with yii2-redis and AWS ElasticCache. Mainly they are:

  • redis redirects are not implemented in yii2-redis
  • multiGet (MGET) only works if cache keys are built a specific way

I thought adding read replicas to the config would prevent redirects. Then I just would have to avoid multiGet.

I'm currently trying to implement the MOVED redis redirect response. This way you won't need to configure read replicas in the config (just use the main endpoint of your AWS cluster) and disable multiGet.

@hofrob hofrob changed the title disable multi get when read replicas are enabled Implement redis redirects (cluster), switch to enable/disable MGET May 4, 2018
@hofrob
Copy link
Contributor Author

hofrob commented May 4, 2018

@samdark @cebe

Anything else I should be doing before you can pull this change?

The only thing I can think of right now, is that there should be an implementation of setting multiple keys in the same hash slot. Then the enableMultiGet would be obsolete.

If there's time, I will take a look at it. But right now, this change works well for us (we're using it on AWS EB with ElasticCache Redis cluster mode enabled).

if (isset($responseParts[2])) {
$this->close();
$this->redirectHostname = $responseParts[2];
$this->open();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to open and close the connection each time a redirect occurs. It would be better to have a pool of connections to choose from and call the method on the correct connection object for the other host.
We have a cluster implementation in Yii core for SQL DB Connection. Maybe it is possible to make a generic multi-connection implementation that works for both SQL clusters and redis clusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Will take a look at it when I have time.

@hofrob
Copy link
Contributor Author

hofrob commented May 20, 2018

Meanwhile we discovered that even multiSet (MSET) throws an error when:

Redis error: CROSSSLOT Keys in request don't hash to the same slot
Redis command was: MSET (...)

Which I will fix first, before looking at the connection pool.

@hofrob hofrob mentioned this pull request Dec 17, 2018
@hofrob
Copy link
Contributor Author

hofrob commented Dec 17, 2018

#178

@hofrob hofrob closed this Dec 17, 2018
samdark pushed a commit that referenced this pull request Nov 5, 2019
@samdark samdark added this to the 2.0.11 milestone Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants