Upgrade SDR to use the RedisClient/RedisClusterClient instead of the legacy Jedis API

[tishun]

(Currently discussing a drastically reduced version of the initial change, see first commit for reference)

Purpose

The purpose of this change is to enable consuming features that are only available in the UnifiedJedis API hierarchy ( including RedisClient/RedisClusterClient APIs that come with Jedis 7.2

Amongst other things we aim to (if possible) handle connection management inside the driver to enable connection-related features, such as client-side caching and client-side geographic failover.

Implementation details

• The first commit represents an approach where we introduce a whole new connection factory and connection class to handle these in a clean slate.
  • The obvious advantages are clean separation and minimal risk of introducing any regressions.
  • The obvious disadvantage is the large amount of new code and maintenance overhead
• The second commit is a stripped down version where we aim for the smallest possible change
  • The obvious advantage is reduced maintenance overhead
  • The obvious disadvantages the possibility to introduce regressions and lack of driver-managed pool

Additional information

• Jedis v6 to v7 migration guide
• Jedis guide (Java)

• You have read the Spring Data contribution guidelines.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

[mp911de]

I'm not sure this is the best way forward. Many command class implementations (such as JedisListCommands) already operate on the level of a JedisBinaryCommands and PipelineBinaryCommands interface. Other types, like JedisStreamCommands can be directly migrated to use e.g. JedisBinaryCommands::xack instead of Jedis::xack.

The problematic ones are in scripting and the move command.
I'm not sure whether it makes sense to introduce an abstraction over JedisConnection and a new UnifiedJedisConnection that can fulfil the invoker contract and how much commonalities there would be between these two classes. Since Jedis and RedisClient do not share UnifiedJedis, we cannot adapt JedisConnection to the new client easily. However, it would be neat to have to construct UnifiedJedis from a Jedis instance using its Connection as input to public UnifiedJedis(ConnectionProvider provider)

Switching cluster connections to UnifiedJedis seems rather straightforward with JedisClusterConnection requiring almost no breaking change.

That being said, it would be a good first step to discuss various approaches before submitting a pull request. We are certainly not adopting another 20kLOC of tech debt.

[tishun]

I'm not sure this is the best way forward. Many command class implementations (such as JedisListCommands) already operate on the level of a JedisBinaryCommands and PipelineBinaryCommands interface. Other types, like JedisStreamCommands can be directly migrated to use e.g. JedisBinaryCommands::xack instead of Jedis::xack.

The problematic ones are in scripting and the move command. I'm not sure whether it makes sense to introduce an abstraction over JedisConnection and a new UnifiedJedisConnection that can fulfil the invoker contract and how much commonalities there would be between these two classes. Since Jedis and RedisClient do not share UnifiedJedis, we cannot adapt JedisConnection to the new client easily. However, it would be neat to have to construct UnifiedJedis from a Jedis instance using its Connection as input to public UnifiedJedis(ConnectionProvider provider)

Switching cluster connections to UnifiedJedis seems rather straightforward with JedisClusterConnection requiring almost no breaking change.

That being said, it would be a good first step to discuss various approaches before submitting a pull request. We are certainly not adopting another 20kLOC of tech debt.

That's fair, I was aiming for a much smaller change but it quickly exploded and then I wanted to prototype something to talk about. I may have let my deadlines get the best of me.

Lets try and discuss this and see what we can do better.

[tishun]

Some general information on the approach in the latest commit:

The JedisConnectionFactory now can work directly with a RedisClient instance, which would enable all connection-based features, but is not completely compatible with the existing JedisConnection contract, specifically:

• since the RedisClient now handles the connection pool internally some operations no longer make sense, for eg. calling select() would select the database for the given connection, but any of the next commands can be executed on any of the underlying connections, so selecting a database is forbidden (outside the configuration of the JedisConnection); similarly setClientName() is also disabled
• the getNativeClient() would return an instance of the RedisClient the contract of the RedisConnection is object, so this is fair, but we can not do a straight replacement, as previously users using this API would have cast the result to Jedis
• watch() would now start a transaction, as the StandardJedisConneciton needs to be bound to a specific connection from the pool, otherwise the watch might get executed on a different connection than the multi()
• most of the other logic remains the same

To preserve complete backwards compatibility we are keeping the old JedisConnection and using it in conjunction with the LegacyJedisAdapter. This allows us to consume the RedisModuleCommands APIs while completely preserving the legacy approach to working with Jedis.

The JedisClientConfiguration uses, by default, the legacy mode. The next major release we can consider changing this to the standard mode, which might cause some breakages (mainly behavioral or related to casting the getNativeConnection()). As a next step we can completely remove the legacy code and switch to using RedisClient

[tishun]

Summary of the last three commits:

Applied comments by @mp911de
The legacy mode of the Jedis connections is not turned on by default and could only be enabled if the user downgrades the Jedis driver to a version that does not have the RedisClient APIs. We greatly simplify the logic and enable the usage of the new APIs by default, but also leave out an option for existing users to fall back.

Sentinel APIs migrated
We are now building up the Sentinel connections using the new Sentinel APIs. Similar to the standalone connections we have a fallback mechanism based on the driver version.
We still depend on some legacy APIs underneath, but we are not exposing them to the end user and have the option to fix that with later submits.

Cluster APIs migrated
Now that we have the UnifiedJedis as a baseline we have the option to also inherit most of the logic of the JedisCluster*Commands from the Jedis*Commands, with the exception where cluster-specific logic is implemented. This greatly improves code reuse.

Replaced the JedisCluster with the UnifiedJedis (as interface) and the RedisClusterClient (as implementation).