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

Multiserver #62

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

ThijsFeryn
Copy link

Hi Colin,

I noticed that Cm_Cache_Backend_Redis can only connect to a single Redis server. For larger setups, it would be nice if we could leverage the Credis_Cluster features.

This pull requests offers Credis_Cluster support with backwards compatibility for the current configuration syntax.

Could you please review the pull request and give me feedback? Could you also explain how I can actually test this on a recent Magento installation where Cm_Cache_Backend_Redis is already there by default?

Cheers
Thijs

}
$this->_redis = new Credis_Cluster($servers);
foreach($this->_redis->clients() as $client) {
$this->_initRedisConfig($client,$options['servers']);
Copy link
Owner

Choose a reason for hiding this comment

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

Should the second argument to _initRedisConfig not be just $options?

Copy link
Author

Choose a reason for hiding this comment

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

That should indeed be the case. I do believe that this is not sufficient. The "_initRedisConfig" method is a way of compensating for the limited amount of options you can pass to the constructor of the Credis_Client or Credis_Cluster.

As you mentioned most of the work should be done in the Credis library. I'll try to figure out how we can do that and send the right pull requests.

@colinmollenhour
Copy link
Owner

After reviewing briefly it seems that there should be other changes to handle the cluster mode properly.

  • Connections should be initialized lazily. Perhaps this is overrated though since in Magento it is likely that all connections will be used on all pages anyway. Yet, it does seem wasteful to open connections that may not be used.
  • The crc32 checksum algorithm is poorly suited to key-ring hashing since it is not a uniform distribution algorithm. It should therefore be replaced with md5 even though md5 is slower because clustering is useless if the distribution is going to be poor to begin with. This is not an issue with your patch of course, just the Credis_Cluster class.
  • Special cases like cleaning should apply to all cluster nodes, but currently it will only apply to one node due to the logic in Credis_Cluster#__call method. Ideally it could be done so that these multi-node operations are done in parallel or in a non-blocking method. Again, this would mostly require changes to Credis_Cluster.

Overall, I think you are heading in the right direction, but the Credis_Cluster class needs the most work.

Thanks for the PR!

@ThijsFeryn
Copy link
Author

You're absolutely right, Colin. A couple of things should be done to fix this issue.

You mentioned the distribution algorithm. I'm not an expert on that matter, but I'm going to take your word for it.

I would also like to extend the constructor to allow more options. Alle the stuff I configure in "_initRedisConfig" should in fact be handeled by the Credis_Client constructor.

And I can definitely live with the fact that connections should be lazy loaded. The question is: should we send each "write" request to all nodes or just some specific "cleaning" actions as you mentioned.

I'm willing to put in some work and building some sort of prototype. I'm quite busy, so this might take some time.

Thanks for taking the time. I surely hope we can find a good way to allow multi-server Redis support in Magento.

@colinmollenhour
Copy link
Owner

The question is which multi-server configuration do you want? Replication or cluster?

Replication: one master (read/write) and one or more slaves (read-only), all containing mirror copies of the full dataset. Good for scaling read capacity.
Cluster: two or more fully independent nodes, each containing a fraction of the full dataset. Good for scaling reads and scaling capacity. Could be useful even on a single machine for scaling reads by using more CPU cores.

Both could be implemented, but Credis_Cluster is only really applicable to the Cluster scenario. The Cluster method would be interesting, but if I personally actually had a need for more than a single Redis instance can handle I'd probably look into using MongoDb first.

@ftsf
Copy link

ftsf commented Sep 10, 2014

While scaling load is one issue, I think a more important issue is eliminating a single point of failure.
Should a redis instance go down, your Magento site goes down. By allowing multiple redis servers that each have a full set of the data, in the event of one failure the other instance can continue serving requests. Does this PR deal with this use case?

@ThijsFeryn
Copy link
Author

@ftsf It does actually. The initial multi-server feature did hash-based sharding. This was built for load scaling and didn't contain replicas of your data.

My pull request features master/slave replication and service discovery using Sentinel. This means that Sentinel monitors your (replicated) Redis instances. If one of the nodes goes down (including the master), this node will no longer be selected for queries and in case a master dies, a new master is elected for write commands.

Hope that answers your question.

@colinmollenhour
Copy link
Owner

Sorry I have not had time to test and merge this.. @ThijsFeryn is this a complete pull request in your opinion? Have you been using this in production?

@ThijsFeryn
Copy link
Author

No, this one is far from complete. And I'm sorry @ftsf for the confusion. I thought that your comment was referring to the Credis library, but instead you're talking about the implementation in Cm_Cache_Backend_Redis.

This is not ready to merge yet. But when it is done, all features I mentioned in the previous comment will be support.

Stay tuned ;-)

@Xon
Copy link
Contributor

Xon commented Feb 1, 2015

@ThijsFeryn are you still working on this?

Is this mostly feature complete and just requires debugging/testing? Or is additional work required.

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.

4 participants