Copilot commented on code in PR #3874:
URL: https://github.com/apache/hertzbeat/pull/3874#discussion_r2569281286
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java:
##########
@@ -214,16 +216,21 @@ private StatefulRedisConnection<String, String>
getSingleConnection(RedisProtoco
* @param redisProtocol protocol
* @return connection map
*/
- private Map<String, StatefulRedisClusterConnection<String, String>>
getConnectionList(RedisProtocol redisProtocol) throws GeneralSecurityException,
IOException {
+ private Map<String, StatefulRedisConnection<String, String>>
getConnectionList(RedisProtocol redisProtocol) throws GeneralSecurityException,
IOException {
// first connection
StatefulRedisClusterConnection<String, String> connection =
getClusterConnection(redisProtocol);
Partitions partitions = connection.getPartitions();
- Map<String, StatefulRedisClusterConnection<String, String>>
clusterConnectionMap = new HashMap<>(partitions.size());
+ Map<String, StatefulRedisConnection<String, String>>
clusterConnectionMap = new HashMap<>(partitions.size());
for (RedisClusterNode partition : partitions) {
RedisURI uri = partition.getUri();
- redisProtocol.setHost(uri.getHost());
- redisProtocol.setPort(String.valueOf(uri.getPort()));
- StatefulRedisClusterConnection<String, String> clusterConnection =
getClusterConnection(redisProtocol);
+ RedisProtocol singleRedisProtocol = RedisProtocol.builder()
+ .host(uri.getHost())
+ .port(String.valueOf(uri.getPort()))
+ .username(redisProtocol.getUsername())
+ .password(redisProtocol.getPassword())
+ .pattern(SINGLE)
Review Comment:
The `singleRedisProtocol` builder is missing the `sshTunnel` field from the
original `redisProtocol`. If SSH tunneling is configured for the cluster
connection, individual node connections will fail to use the tunnel, causing
connection failures. Add `.sshTunnel(redisProtocol.getSshTunnel())` to the
builder.
```suggestion
.pattern(SINGLE)
.sshTunnel(redisProtocol.getSshTunnel())
```
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java:
##########
@@ -140,7 +142,7 @@ private Map<String, String> getSingleRedisInfo(Metrics
metrics) throws GeneralSe
* @return data
*/
private List<Map<String, String>> getClusterRedisInfo(Metrics metrics)
throws GeneralSecurityException, IOException {
- Map<String, StatefulRedisClusterConnection<String, String>>
connectionMap = getConnectionList(metrics.getRedis());
+ Map<String, StatefulRedisConnection<String, String>> connectionMap =
getConnectionList(metrics.getRedis());
Review Comment:
The change from `StatefulRedisClusterConnection` to
`StatefulRedisConnection` lacks test coverage. The existing test
`RedisClusterCollectImplTest.java` will need to be updated to mock the new
connection flow through `getSingleConnection()` instead of directly using
cluster connections.
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/redis/RedisCommonCollectImpl.java:
##########
@@ -214,16 +216,21 @@ private StatefulRedisConnection<String, String>
getSingleConnection(RedisProtoco
* @param redisProtocol protocol
* @return connection map
*/
- private Map<String, StatefulRedisClusterConnection<String, String>>
getConnectionList(RedisProtocol redisProtocol) throws GeneralSecurityException,
IOException {
+ private Map<String, StatefulRedisConnection<String, String>>
getConnectionList(RedisProtocol redisProtocol) throws GeneralSecurityException,
IOException {
// first connection
StatefulRedisClusterConnection<String, String> connection =
getClusterConnection(redisProtocol);
Partitions partitions = connection.getPartitions();
- Map<String, StatefulRedisClusterConnection<String, String>>
clusterConnectionMap = new HashMap<>(partitions.size());
+ Map<String, StatefulRedisConnection<String, String>>
clusterConnectionMap = new HashMap<>(partitions.size());
for (RedisClusterNode partition : partitions) {
RedisURI uri = partition.getUri();
- redisProtocol.setHost(uri.getHost());
- redisProtocol.setPort(String.valueOf(uri.getPort()));
- StatefulRedisClusterConnection<String, String> clusterConnection =
getClusterConnection(redisProtocol);
+ RedisProtocol singleRedisProtocol = RedisProtocol.builder()
+ .host(uri.getHost())
+ .port(String.valueOf(uri.getPort()))
+ .username(redisProtocol.getUsername())
+ .password(redisProtocol.getPassword())
Review Comment:
The `singleRedisProtocol` builder is missing the `timeout` field from the
original `redisProtocol`. This could cause connections to cluster nodes to use
default timeout values instead of the configured timeout, potentially leading
to unexpected connection behavior. Add `.timeout(redisProtocol.getTimeout())`
to the builder.
```suggestion
.password(redisProtocol.getPassword())
.timeout(redisProtocol.getTimeout())
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
