langchain icon indicating copy to clipboard operation
langchain copied to clipboard

Support Redis Sentinel database connections

Open sseide opened this issue 1 year ago • 5 comments

Support Redis Sentinel database connections

This PR adds the support to connect not only to Redis standalone servers but High Availability Replication sets too (https://redis.io/docs/management/sentinel/) Redis Replica Sets have on Master allowing to write data and 2+ replicas with read-only access to the data. The additional Redis Sentinel instances monitor all server and reconfigure the RW-Master on the fly if it comes unavailable.

Therefore all connections must be made through the Sentinels the query the current master for a read-write connection. This PR adds basic support to also allow a redis connection url specifying a Sentinel as Redis connection.

Before submitting

Redis documentation and Jupyter notebook with Redis examples are updated to mention how to connect to a redis Replica Set with Sentinels

Who can review?

Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested:

VectorStores / Retrievers / Memory - @dev2049 -

Remark - i did not found test cases for Redis server connections to add new cases here. Therefor i tests the new utility class locally with different kind of setups to make sure different connection urls are working as expected. But no test case here as part of this PR.

sseide avatar May 24 '23 13:05 sseide

@tylerhutcherson any thoughts?

hwchase17 avatar May 24 '23 15:05 hwchase17

@DvirDukhan With current implementation it is not possible to define multiple sentinel instances unfortunately. It might be possible not using urllib Parser using a comma separated list as host:port names or similar but it does not look right...

Usage implications

  • Using a classic (e.g. bare metal) setup you will have a problem if the one sentinel mentioned within the url is down.
  • Having a load balancer in front of the sentinels (only for the clients) it will work even when one sentinel goes down as all others are reachable with the same DNS name - this kind of setup is quite common using most public helm charts to setup Redis in Kubernetes - there you have a load-balanced DNS name as well as one name per Sentinel. Redis-Clients using the load-balanced name can reach Sentinels all the time to query the current master Redis server. So it is not a problem for HA setups.

Having said this i know this implementation is far from perfect and i am open for enhancements... Especially i'd like to hear about Redis Labs thoughts for Urls with Sentinels. It would be great to have some authoritive suggestions on that to get common implementations over different languages.

For this PR i tried to get an implementations without changes needed to existing clients and where a Sentinel setup can be used as a drop-in replacement without code changes for everyone using Redis standalone right now - just change the url configuration and you are ready to go.

Testing

i have not found test files regarding the Redis connection setup, therefor no changes here. And all current test for Redis standalone must work without changes, nothing needed here.

Improvements needed

Current PR does not address Sentinel with TLS setup. But i like to address this as well.

Should the TLS Url changed to "rediss+sentinel" similar to Redis standalone? Define TLS via Url parameter? Shall we support a mixed setup with Redis server or Sentinel using TLS and the other not? Server-side admin can be configure this kind of setup but - at least from my point of view - security wise it is broken and should not be acceptable for clients, So i tend to say either no TLS at all or all connections use TLS.

What are your thoughts about that?

sseide avatar May 29 '23 08:05 sseide

@DvirDukhan any hope of this one making it into a release soon? Also several other redis based modules could make good use of the provided get_client util.

jakobsa avatar Jun 09 '23 09:06 jakobsa

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Jul 11, 2023 2:46pm

vercel[bot] avatar Jul 11 '23 14:07 vercel[bot]

as there was no remarks with preferences about how to handle TLS encryption I added the uri scheme rediss+sentinel:// similar to the already existing rediss:// url scheme. As all these urls are only valid for straight setups without every possible combination all TLS settings configured (certificates, OCPS, ...) are reused for the Redis Sentinel as well as the Redis Server connection.

If someone needs special setups he is always free to provide an already preconfigured redis client.

@hwchase17 @DvirDukhan @Spartee Any chance this gets merged soon? Any other remarks or wishes?

sseide avatar Jul 11 '23 14:07 sseide

@sseide looks good to me, thanks for the addition!

baskaryan avatar Jul 17 '23 14:07 baskaryan