Implemented read-write separation based on JedisSentineled #4231
Conversation
There was a problem hiding this comment.
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
force-pushed
the
sentineledSlavePool
branch
from
8068e3b to
7910819
Compare
|
@ggivo OK, I modified the submitted code, please review it again |
There was a problem hiding this comment.
@koleter
Thanks for working on this. A couple of important points:
- Backward compatibility
We must preserve existing behavior. By default, Jedis should continue to read from upstream (master) nodes only. I suggest introducing aReadFromstrategy 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.
- 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).
| private void initSlaves(List<HostAndPort> slaves) { | ||
| List<PoolInfo> removedSlavePools = new ArrayList<>(); | ||
| try { | ||
| synchronized (slavePools) { |
There was a problem hiding this comment.
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. (#3480)
We should avoid introducing new synchronized sections here, and instead rely on other type syncronisation.
| public Connection getConnection(CommandArguments args) { | ||
| boolean readCommand = masterClientConfig.isReadCommand(args); | ||
| if (readCommand) { | ||
| Connection slaveConn = getSlaveResource(); | ||
| if (slaveConn != null) { | ||
| return slaveConn; | ||
| } | ||
| if (!masterClientConfig.isFallbackToMaster()) { | ||
| throw new JedisException("can not get Connection, all slave is invalid"); | ||
| } | ||
| } | ||
| return pool.getResource(); | ||
| } |
There was a problem hiding this comment.
See my comment about preserving backword compatibility
|
|
||
| /** | ||
| * fallback when no replicas are healthy, default to master | ||
| * @return {@code true} - to execute command by master. {@code false} - throw exception. | ||
| */ | ||
| default boolean isFallbackToMaster() { | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * check a Command is READONLY | ||
| * @param args | ||
| * @return | ||
| */ | ||
| default boolean isReadCommand(CommandArguments args) { | ||
| return Commands.ReadOnlyCommands.contains(args.getCommand()); | ||
| } |
There was a problem hiding this comment.
We are introducing support for READ commands only for the JedisSentineled client.
Because of this, the read-from settings should be configured specifically in JedisSentineled (or its SentineledConnectionProvider) rather than being placed in the generic client configuration.
In addition, I think it is better to determine whether a command is a READ command by exposing a configurable Predicate. This gives users flexibility and avoids the need to override methods. I like how this is achieved in Lettuce:
• Predicate interface: ReadOnlyPredicate
• Example implementation: ReadOnlyCommands
• Configuration: ClientOptions
| } | ||
| } | ||
| }, "+switch-master"); | ||
| }, "+switch-master", "+sdown", "-sdown", "+slave"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
And +odown will only be triggered when the master goes down, my redis version is 6.2.6
| List<PoolInfo> removedSlavePools = new ArrayList<>(); | ||
| try { | ||
| synchronized (slavePools) { | ||
| Loop: |
There was a problem hiding this comment.
nit: The nested loop with continue Loop reduces readability .
- 新增 ReadFrom 枚举类，用于配置读取策略 - 在 JedisSentineled 和 SentineledConnectionProvider 中添加读取策略相关代码 - 修改 DefaultJedisClientConfig，移除不必要的 fallbackToMaster配置 - 重构 readOnlyCommands 判断逻辑，提高可维护性
|
@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. |
# Conflicts: # src/test/java/redis/clients/jedis/SentineledConnectionProviderTest.java
Implemented read-write separation based on JedisSentineled