Implemented read-write separation based on JedisSentineled

[koleter]

Implemented read-write separation based on JedisSentineled

[ggivo]

This is a good idea 👍 — adding explicit awareness of read-only commands will definitely help not only for sentinel but also for replica/cluster use cases.

One suggestion: rather than hardcoding the classification, you could follow an approach similar to Lettuce’s ReadOnlyCommands and make it configurable via something like ClientOptions.

That way:
• The default set of read-only commands can be maintained centrally.
• Users can override/extend the classification if needed (e.g. for modules or new commands).
• It handles tricky cases like FUNCTION where subcommands mix read/write semantics.

Suggested next steps:
1. Introduce a ReadOnlyCommands (or similar) class to hold the default command/subcommand definitions.
2. Add configuration hooks in JedisClientConfig (or equivalent) so users can supply their own overrides/extensions.
3. Update the client logic to consult this configurable set whenever deciding if a command is read-only.

[koleter]

@ggivo OK, I modified the submitted code, please review it again

[ggivo]

@koleter
Thanks for working on this. A couple of important points:

1. Backward compatibility
   We must preserve existing behavior. By default, Jedis should continue to read from upstream (master) nodes only. I suggest introducing a ReadFrom strategy enum (similar to Lettuce’s ReadFrom settings):
   • ReadFrom.UPSTREAM – default, read only from upstream/master.
   • ReadFrom.REPLICA – read only from replicas.
   • ReadFrom.UPSTREAM_PREFERRED – prefer upstream, fallback to replica if upstream unavailable.
   • ReadFrom.REPLICA_PREFERRED – prefer replica, fallback to upstream if no replica available.

This ensures backward compatibility while giving users explicit control.

2. Virtual threads
   synchronized blocks cause carrier thread pinning, which undermines Loom’s scalability model. To avoid this, we replaced synchronized blocks with ReentrantLock in other parts of the driver we should avoid introducing new synchronized sections here, and instead rely on ReentrantLock (or another Loom-compatible concurrency primitive).

[ggivo]

Based on High availability with Redis Sentinel

+sentinel <instance details> -- A new sentinel for this master was detected and attached.
+sdown <instance details> -- The specified instance is now in Subjectively Down state.
-sdown <instance details> -- The specified instance is no longer in Subjectively Down state.
+odown <instance details> -- The specified instance is now in Objectively Down state.
-odown <instance details> -- The specified instance is no longer in Objectively Down state.

Not clear the difference between sdown/odown events. If I am reading it right odown events are send when majority of sentinels consider the node down, and sdown is send when one sentinel considers the node down.

Probably we should react on odown events.

[koleter]

There is a problem here. When I kill a redis node, +odown is triggered, but when the node comes back online, there is no -odown event.

[koleter]

And +odown will only be triggered when the master goes down, my redis version is 6.2.6

[koleter]

@ggivo The majority opinion has been revised, but there are some situations regarding sdown/odown events that may need to remain as is.

[ggivo]

@ggivo The majority opinion has been revised, but there are some situations regarding sdown/odown events that may need to remain as is.

Sorry for the late reply. I will take a look at it until the end of the week.

[koleter]

@ggivo Running the test cases is taking too long. Could you please check if the current modifications are feasible?

[ggivo]

@koleter
Hey, thanks for moving this forward.
We are a bit stretched currently, wrapping up some work 8.4 release, will do my best to provide feedback early next week

[koleter]

Hi maintainers,
It looks like the GitHub Actions workflow is awaiting approval.
Could you please approve and trigger the CI when you have time? Thanks!

[koleter]

@ggivo

the failure in Build and Test (8.2) seems unrelated to the PR changes and is caused by a Maven Central connection reset during dependency:go-offline (OSGi artifacts).
Could you please help re-run the workflow?

Thanks a lot!

[koleter]

Hi maintainers,
when you have time, could you please help take a look at this PR?
Thanks a lot!

[ggivo]

@koleter
First of all, sorry for the slow response here.
The team has been quite stretched recently, which has delayed our review, but we’ll do our best to provide feedback as soon as we have some time.

Really appreciate the effort and persistence you’ve put into this PR — thank you for sticking with it!

[jit-ci]

❌ Security scan failed

Security scan failed: Branch sentineledSlavePool does not exist in the remote repository

---

💡 Need to bypass this check? Comment @sera bypass to override.