search

overleaf / redis-wrapper

a wrapper around redis which detects if it should use the normal redis driver or redis-sentinel
3 stars 4 forks source link

health check broken for redis cluster #12

Closed das7pad closed 4 years ago

das7pad commented 5 years ago

The new health check from #11 changes the health check from a simple ping to write/read/delete operations.

This breaks the usage of a redis cluster. Redis clusters shard the hash space and grant only a single node write access on each shard. Writing on an arbitrary node into a slot that it does not have write capabilities for, results in an error - e.g. MOVED 8647 DELEGATED_IP_ADDRESS:6379.

The health check would try to write a new key per cluster member. Chances to hit a delegated hash slot for every node on every check round are close to 0. But it will break for sure with primary/secondary setups - secondaries never get write access until a primary dies and they get promoted.

jdleesmiller commented 5 years ago

Thanks, good point. We aren't using redis cluster any more, so we didn't try to keep it working. It's not something we officially support, so we may not fix this, sorry.

das7pad commented 5 years ago

We can leverage the cluster-require-full-coverage option of the redis cluster, which defaults to yes. It essentially blocks writes as soon as the cluster has lost any primary that it can not replace with a secondary.

Running the heath check on any node is sufficient then.

I patched it already for my infra, would you accept a PR for it?

jdleesmiller commented 5 years ago

Thanks, good to know about the workaround. I don't think we have any plans to support redis cluster going forward, so I don't think we'd accept a PR for this at this time, unfortunately.