feat(session): a unified Redis session client that supports switching between underlying clients #611
Conversation
# Conflicts: # agentscope-dependencies-bom/pom.xml
|
|
|
|
Summary of ChangesHello @benym, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Redis session management within AgentScope by introducing a unified RedisSession client. This new implementation leverages an adapter pattern to abstract away the underlying Redis client library, enabling seamless integration with Jedis, Lettuce, and Redisson, and supporting diverse Redis deployment topologies like standalone, cluster, and sentinel. This change streamlines the session management API, reduces code duplication, and improves flexibility for users operating in various production environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
ptal,thx @AlbumenJ @chickenlj |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified RedisSession client, which is a significant improvement for supporting various Redis clients (Jedis, Lettuce, Redisson) and deployment modes. The use of the adapter pattern is well-executed. I've identified a few areas for improvement, primarily concerning performance and documentation accuracy. The Jedis and Lettuce adapters currently use the KEYS command, which can pose a performance risk in production; I've suggested switching to SCAN. Additionally, there are some inaccuracies in Javadoc and README examples that should be corrected for clarity. Overall, this is a great feature addition.
| public Set<String> findKeysByPattern(String pattern) { | ||
| return new HashSet<>(unifiedJedis.keys(pattern)); | ||
| } |
There was a problem hiding this comment.
The findKeysByPattern method uses the keys() command, which can block the Redis server and severely impact performance, especially in production environments with a large number of keys. It is strongly recommended to use the SCAN command for safer, non-blocking key iteration.
public Set<String> findKeysByPattern(String pattern) {
Set<String> matchingKeys = new HashSet<>();
String cursor = redis.clients.jedis.params.ScanParams.SCAN_POINTER_START;
redis.clients.jedis.params.ScanParams scanParams = new redis.clients.jedis.params.ScanParams().match(pattern);
do {
redis.clients.jedis.resps.ScanResult<String> scanResult = unifiedJedis.scan(cursor, scanParams);
matchingKeys.addAll(scanResult.getResult());
cursor = scanResult.getCursor();
} while (!cursor.equals(redis.clients.jedis.params.ScanParams.SCAN_POINTER_START));
return matchingKeys;
}| public Set<String> findKeysByPattern(String pattern) { | ||
| List<String> keysList = commands.keys(pattern); | ||
| return new HashSet<>(keysList); | ||
| } |
There was a problem hiding this comment.
The findKeysByPattern method uses the keys() command, which can block the Redis server and should be avoided in production. Please use the SCAN command instead, which allows for iterative scanning without blocking and is safer for production environments.
public Set<String> findKeysByPattern(String pattern) {
Set<String> keys = new HashSet<>();
io.lettuce.core.ScanCursor cursor = io.lettuce.core.ScanCursor.INITIAL;
while (!cursor.isFinished()) {
io.lettuce.core.KeyScanCursor<String> scanResult = commands.scan(cursor, io.lettuce.core.ScanArgs.Builder.match(pattern));
keys.addAll(scanResult.getKeys());
cursor = scanResult;
}
return keys;
}| Mono<Long> deletedCount = session.clearAllSessions(); | ||
| long count = deletedCount.block(); |
There was a problem hiding this comment.
The example for clearAllSessions is incorrect. The method returns a Mono<Integer>, not Mono<Long>. Consequently, the result should be stored in an Integer or int.
| Mono<Long> deletedCount = session.clearAllSessions(); | |
| long count = deletedCount.block(); | |
| Mono<Integer> deletedCount = session.clearAllSessions(); | |
| long count = deletedCount.block(); |
| Mono<Long> deletedCount = session.clearAllSessions(); | ||
| long count = deletedCount.block(); |
There was a problem hiding this comment.
The example for clearAllSessions is incorrect. The method returns a Mono<Integer>, not Mono<Long>. Consequently, the result should be stored in an Integer or int.
| Mono<Long> deletedCount = session.clearAllSessions(); | |
| long count = deletedCount.block(); | |
| Mono<Integer> deletedCount = session.clearAllSessions(); | |
| long count = deletedCount.block(); |
| * <p><strong>Jedis Usage Examples:</strong></p> | ||
| * | ||
| * <p>Jedis Standalone (using RedisClient): | ||
| * | ||
| * <pre>{@code | ||
| * // Create Jedis RedisClient (new API) | ||
| * RedisClient redisClient = RedisClient.create("redis://localhost:6379"); | ||
| * | ||
| * // Build RedisSession | ||
| * Session session = RedisSession.builder() | ||
| * .redisClient(redisClient) | ||
| * .build(); | ||
| * }</pre> | ||
| * | ||
| * <p>Jedis Cluster (using RedisClusterClient): | ||
| * | ||
| * <pre>{@code | ||
| * // Create Jedis RedisClusterClient | ||
| * Set<HostAndPort> nodes = new HashSet<>(); | ||
| * nodes.add(new HostAndPort("localhost", 7000)); | ||
| * nodes.add(new HostAndPort("localhost", 7001)); | ||
| * nodes.add(new HostAndPort("localhost", 7002)); | ||
| * RedisClusterClient redisClusterClient = RedisClusterClient.create(nodes); | ||
| * | ||
| * // Build RedisSession | ||
| * Session session = RedisSession.builder() | ||
| * .redisClusterClient(redisClusterClient) | ||
| * .build(); | ||
| * }</pre> | ||
| * | ||
| * <p>Jedis Sentinel (using RedisSentinelClient): | ||
| * | ||
| * <pre>{@code | ||
| * // Create Jedis RedisSentinelClient | ||
| * Set<String> sentinelNodes = new HashSet<>(); | ||
| * sentinelNodes.add("localhost:26379"); | ||
| * sentinelNodes.add("localhost:26380"); | ||
| * RedisSentinelClient redisSentinelClient = RedisSentinelClient.create("mymaster", sentinelNodes); | ||
| * | ||
| * // Build RedisSession | ||
| * Session session = RedisSession.builder() | ||
| * .redisSentinelClient(redisSentinelClient) | ||
| * .build(); | ||
| * }</pre> |
There was a problem hiding this comment.
The Javadoc examples for building a RedisSession with Jedis are incorrect. The builder does not have methods like redisClient(), redisClusterClient(), or redisSentinelClient(). It has a single jedisClient(UnifiedJedis) method that should be used for all Jedis deployment modes (standalone, cluster, sentinel). Please update the examples to reflect the correct API usage, similar to what's in the README.
* <p><strong>Jedis Usage Examples:</strong></p>
*
* <p>Jedis Standalone (using RedisClient):
*
* <pre>{@code
* // Create Jedis RedisClient (new API)
* redis.clients.jedis.RedisClient redisClient = redis.clients.jedis.RedisClient.create("redis://localhost:6379");
*
* // Build RedisSession
* Session session = RedisSession.builder()
* .jedisClient(redisClient)
* .build();
* }</pre>
*
* <p>Jedis Cluster (using RedisClusterClient):
*
* <pre>{@code
* // Create Jedis RedisClusterClient
* Set<redis.clients.jedis.HostAndPort> nodes = new HashSet<>();
* nodes.add(new redis.clients.jedis.HostAndPort("localhost", 7000));
* nodes.add(new redis.clients.jedis.HostAndPort("localhost", 7001));
* nodes.add(new redis.clients.jedis.HostAndPort("localhost", 7002));
* redis.clients.jedis.RedisClusterClient redisClusterClient = redis.clients.jedis.RedisClusterClient.create(nodes);
*
* // Build RedisSession
* Session session = RedisSession.builder()
* .jedisClient(redisClusterClient)
* .build();
* }</pre>
*
* <p>Jedis Sentinel (using RedisSentinelClient):
*
* <pre>{@code
* // Create Jedis RedisSentinelClient
* Set<String> sentinelNodes = new HashSet<>();
* sentinelNodes.add("localhost:26379");
* sentinelNodes.add("localhost:26380");
* redis.clients.jedis.RedisSentinelClient redisSentinelClient = redis.clients.jedis.RedisSentinelClient.create("mymaster", sentinelNodes);
*
* // Build RedisSession
* Session session = RedisSession.builder()
* .jedisClient(redisSentinelClient)
* .build();
* }</pre>| public List<String> rangeList(String key, long start, long end) { | ||
| RList<String> rList = redissonClient.getList(key, StringCodec.INSTANCE); | ||
| return rList.range((int) start, (int) end); | ||
| } |
There was a problem hiding this comment.
The rangeList method casts long indices to int for the call to rList.range(). This could lead to an integer overflow and incorrect behavior if the list size or indices exceed Integer.MAX_VALUE. While Redisson's RList API is limited to int indices, it would be safer to validate the input long values to prevent silent truncation and potential bugs.
public List<String> rangeList(String key, long start, long end) {
if (start > Integer.MAX_VALUE || end > Integer.MAX_VALUE || start < Integer.MIN_VALUE || end < Integer.MIN_VALUE) {
throw new IllegalArgumentException("Index out of range for Redisson RList, which supports int-based indexing.");
}
RList<String> rList = redissonClient.getList(key, StringCodec.INSTANCE);
return rList.range((int) start, (int) end);
}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| * @author Kevin | ||
| * @author jianjun.xu | ||
| * @author benym |
There was a problem hiding this comment.
Please remove these tags :)
| * @author Kevin | ||
| * @author jianjun.xu | ||
| * @author benym | ||
| * @since 1.0.8 |
There was a problem hiding this comment.
Please remove these tags :)
| */ | ||
| public class LettuceClientAdapter implements RedisClientAdapter { | ||
|
|
||
| private final io.lettuce.core.RedisClient redisClient; |
|
done. please check again. @AlbumenJ |
| * </ul> | ||
| */ | ||
| @Deprecated | ||
| public class JedisSession implements Session { |
There was a problem hiding this comment.
Can we reuse this class?
There was a problem hiding this comment.
Can we reuse this class?
This class is unlikely to be reused. JedisPool in JedisSession can be replaced with UnifiedJedis to adapt to various Redis deployment environments, but I don't think it's necessary to maintain this class anymore. This would allow for multiple parallel implementations of Redis-based sessions, and the core logic of session-based systems is clearly repetitive. The core logic of RedisSeesion is based on JedisSeesion, using an adapter design to hide the underlying client operations. I believe that for users, Jedis is just a client; they are actually operating Redis. Therefore, a unified RedisSeesion is sufficient. If Jedis is needed, the switch can be completed by specifying the Jedis client during initialization. Now that everything can be unified under a single RedisSession, the old implementation can be deprecated, reducing maintenance burden.
There was a problem hiding this comment.
From a design perspective, the adapter handles the client commands for operating Redis, while existing JedisSession and RedissonSession handle session operation logic. This part is largely reusable and doesn't need to be implemented repeatedly for the client.
2 participants
AgentScope-Java Version
1.0.8-SNAPSHOT
Description
releted to #610
Background
The current JedisSeesion uses JedisPool, which only supports standalone scenarios. In production environments, users mostly use more complex deployment methods, such as sentinels, clusters, and master-slave setups. Furthermore, in a Java environment, users may use different Redis client implementations, such as
Jedis,Lettuce, andRedisson. If each is used independently, scaling up with different Redis client implementations would result ina lot of duplicate code. Moreover, naming conventions like JedisSeesion are unclear, as they are all essentially types of Redis clients.New Feature
agentscope-extensions-session-redismoduleThe newly added codes all have complete javadoc documentation and the
mvn testhas passed.Modify
New Usage
Jedis Usage Examples
Lettuce Usage Examples
Redisson Usage Example
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)