Implemented read-write separation based on sentinel

[koleter]

No description provided.

[koleter]

Implemented read-write separation based on sentinel

[ggivo]

Hi @koleter

Thanks for opening this PR! Could you please provide more context about the issue it addresses? It would be helpful if you could open a corresponding GitHub issue describing the use case or bug you’re encountering.

This will give us a common place to discuss the problem and proposed solution before reviewing the code in detail.

If the PR is still a work in progress, please mark it as a draft. Once it’s ready for review, you can mark it as ready for review to notify the team.

Thanks again for your contribution — looking forward to your input!

[koleter]

We have some redis servers that we plan to access through Sentinel. Currently, we use read-write separation to access them. If we use Jedis directly, all traffic will be sent to the master node, which may cause redis to crash.

[koleter]

Then we can access redis like this

package org.example.jedis;

import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.JedisSentinelPool;
import redis.clients.jedis.JedisSentinelSlavePool;
import redis.clients.jedis.Protocol;

import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;


public class SentinelDemo {

    private static final Logger log = LoggerFactory.getLogger(SentinelDemo.class);

    public static void main(String[] args) throws InterruptedException {
        String password = "0a2eb141353cf115";

        String masterName = "mymaster";

        Set<String> sentinels = new HashSet<>();
        sentinels.add("10.148.17.43:26379"); 
        sentinels.add("10.148.17.43:26380"); 
        sentinels.add("10.148.17.43:26381"); 


        int threads = 10;

        GenericObjectPoolConfig<Jedis> poolConfig = new GenericObjectPoolConfig<>();
        poolConfig.setMaxTotal(threads);

        ExecutorService threadPool = Executors.newFixedThreadPool(threads);

        // create Jedis master pool
        JedisSentinelPool sentinelPool = new JedisSentinelPool(masterName, sentinels, poolConfig, Protocol.DEFAULT_TIMEOUT, password);

        for (int i = 0; i < threads; i++) {
            threadPool.submit(() -> {
                while (true) {
                    try(Jedis jedis = sentinelPool.getResource()) {
                        jedis.set("key", "value");
                    } catch (Exception e) {
                    }
                }
            });
        }

        ExecutorService threadPool1 = Executors.newFixedThreadPool(threads);
        // create Jedis slave pool
        JedisSentinelSlavePool sentinelSlavePool = new JedisSentinelSlavePool(masterName, sentinels, poolConfig, Protocol.DEFAULT_TIMEOUT, password);

        for (int i = 0; i < threads; i++) {
            threadPool1.submit(() -> {
                while (true) {
                    try(Jedis jedis = sentinelSlavePool.getResource()) {
                        jedis.get("key");
                    } catch (Exception e) {
                    }
                }
            });
        }
        
        threadPool.awaitTermination(1000, TimeUnit.DAYS);
    }
}

[koleter]

@ggivo How can I mark this pr as ready for review?

[ggivo]

@koleter
When you open a new PR, it can be marked as DRAFT and when ready, remove the DRAFT so whoever is interested can review it. This one is not marked as a draft, so it is fine.

Have in mind that the team is a bit stretched right now, and it might take some time before we can review it.
I will spend some time on it this week for initial review, to make sure it is safe and enable at least the pipeline tests.

[ggivo]

@koleter
At first glance, the change appears to lack tests

[koleter]

@ggivo ok, I've added some tests

[a-TODO-rov]

Hey @koleter .
Thanks for bringing up this important use case! Read/write separation in Sentinel environments is definitely a valuable feature that many users need.
After reviewing your PR and the current codebase, I'd like to suggest focusing on JedisSentineled - the more modern API rather than extending the legacy JedisSentinelPool. Here's why:
The JedisSentineled (which extends UnifiedJedis) is the modern, recommended approach for Sentinel usage. It has several advantages over the legacy JedisSentinelPool:

• Better Architecture: Uses the provider pattern (SentineledConnectionProvider) which is cleaner and easier to extend. Also the existing getConnection(CommandArguments args) method can be more easily enhanced
• Unified API: Part of the modern UnifiedJedis ecosystem, which is being actively supported and contains all the latest features

I would also like to suggest an approach, which leads to less code duplication (which means easier to support) - instead duplicating the class, integrate the logic in the existing one. Concentrate on the important changes:

• Add slave pool management
• Extend sentinel monitoring to handle slave events

We can leverage intent based connection pooling, where we can get a connection with the intent to read and than a connection to a slave will be provided for us.
I will create a dedicated issue where we can proceed this discussion further.