Copilot commented on code in PR #6250:
URL: https://github.com/apache/shenyu/pull/6250#discussion_r2588036690
##########
shenyu-infra/shenyu-infra-etcd/src/main/java/org/apache/shenyu/infra/etcd/client/EtcdClient.java:
##########
@@ -375,22 +363,58 @@ public GetResponse getRange(final String key, final
GetOption getOption) {
/**
* put data as ephemeral.
- * @param key key
+ *
+ * @param key key
* @param value value
*/
public void putEphemeral(final String key, final String value) {
try {
KV kvClient = client.getKVClient();
kvClient.put(ByteSequence.from(key, UTF_8),
ByteSequence.from(value, UTF_8),
-
PutOption.newBuilder().withLeaseId(globalLeaseId).build())
+ PutOption.newBuilder().withLeaseId(globalLeaseId).build())
.get(timeout, TimeUnit.MILLISECONDS);
} catch (InterruptedException | ExecutionException | TimeoutException
e) {
LOG.error("putEphemeral(key:{},value:{}) error.", key, value, e);
+ throw new ShenyuException(e);
}
}
+ private void initLease() {
+ try {
+ this.globalLeaseId =
client.getLeaseClient().grant(ttl).get().getID();
+ keepAlive();
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.error("initLease error.", e);
+ throw new ShenyuException(e);
+ }
+ }
+
+ private void keepAlive() {
+ client.getLeaseClient().keepAlive(globalLeaseId, new
StreamObserver<>() {
+ @Override
+ public void onNext(final LeaseKeepAliveResponse
leaseKeepAliveResponse) {
+ }
+
+ @Override
+ public void onError(final Throwable throwable) {
+ LOG.error("keep alive error", throwable);
+ try {
+ TimeUnit.SECONDS.sleep(1);
+ } catch (InterruptedException e) {
+ LOG.error("keep alive sleep error", e);
+ }
+ keepAlive();
+ }
+
+ @Override
+ public void onCompleted() {
+ }
+ });
+ }
Review Comment:
The recursive call to `keepAlive()` in the `onError` handler can lead to a
stack overflow if errors occur continuously. The 1-second sleep doesn't prevent
rapid recursive calls from exhausting the stack. Consider using an iterative
approach with a background thread or implementing a circuit breaker pattern to
limit retries.
##########
shenyu-infra/shenyu-infra-redis/src/main/java/org/apache/shenyu/infra/redis/RedisConnectionFactory.java:
##########
@@ -126,10 +135,27 @@ private List<RedisNode> createRedisNode(final String url)
{
List<RedisNode> redisNodes = new ArrayList<>();
List<String> nodes = Lists.newArrayList(Splitter.on(";").split(url));
for (String node : nodes) {
- String[] parts = StringUtils.split(node, ":");
- Assert.state(Objects.requireNonNull(parts).length == 2, "Must be
defined as 'host:port'");
- redisNodes.add(new RedisNode(parts[0],
Integer.parseInt(parts[1])));
+ redisNodes.add(parseRedisNode(node));
}
return redisNodes;
}
+
+ private RedisNode parseRedisNode(final String url) {
+ String host = url;
+ int port = 6379;
+ int lastColonIndex = url.lastIndexOf(":");
+ int bracketIndex = url.lastIndexOf("]");
+ if (bracketIndex > -1) {
+ if (lastColonIndex > bracketIndex) {
+ host = url.substring(0, lastColonIndex);
+ port = Integer.parseInt(url.substring(lastColonIndex + 1));
+ }
+ } else {
+ if (lastColonIndex > -1) {
+ host = url.substring(0, lastColonIndex);
+ port = Integer.parseInt(url.substring(lastColonIndex + 1));
+ }
+ }
Review Comment:
The `parseRedisNode` method lacks error handling for malformed URLs. If
`Integer.parseInt()` fails (e.g., with a non-numeric port like
"localhost:abc"), it will throw a `NumberFormatException` which is not handled.
Additionally, there's no validation that the host is not empty. Consider adding
proper error handling and validation, or wrapping exceptions in a more
informative exception type.
```suggestion
try {
port = Integer.parseInt(url.substring(lastColonIndex +
1));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("Invalid port in
Redis node URL: " + url, e);
}
}
} else {
if (lastColonIndex > -1) {
host = url.substring(0, lastColonIndex);
try {
port = Integer.parseInt(url.substring(lastColonIndex +
1));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("Invalid port in
Redis node URL: " + url, e);
}
}
}
if (host == null || host.trim().isEmpty()) {
throw new IllegalArgumentException("Host is empty in Redis node
URL: " + url);
}
if (port < 1 || port > 65535) {
throw new IllegalArgumentException("Port out of range in Redis
node URL: " + url);
}
```
##########
shenyu-infra/shenyu-infra-redis/src/main/java/org/apache/shenyu/infra/redis/RedisConnectionFactory.java:
##########
@@ -89,12 +92,18 @@ private GenericObjectPoolConfig<?> getPoolConfig(final
RedisConfigProperties red
* @param redisConfigProperties the rate limiter config
* @return the redis standalone configuration
*/
- protected final RedisStandaloneConfiguration
redisStandaloneConfiguration(final RedisConfigProperties redisConfigProperties)
{
+ /**
+ * Redis standalone configuration redis standalone configuration.
+ *
+ * @param redisConfigProperties the rate limiter config
+ * @return the redis standalone configuration
+ */
Review Comment:
Duplicate JavaDoc comments detected. There are two identical JavaDoc blocks
for the same method `redisStandaloneConfiguration`. The first block (lines
89-94) should be removed, keeping only the second one (lines 95-100).
```suggestion
```
##########
shenyu-infra/shenyu-infra-redis/src/main/java/org/apache/shenyu/infra/redis/RedisConnectionFactory.java:
##########
@@ -126,10 +135,27 @@ private List<RedisNode> createRedisNode(final String url)
{
List<RedisNode> redisNodes = new ArrayList<>();
List<String> nodes = Lists.newArrayList(Splitter.on(";").split(url));
for (String node : nodes) {
- String[] parts = StringUtils.split(node, ":");
- Assert.state(Objects.requireNonNull(parts).length == 2, "Must be
defined as 'host:port'");
- redisNodes.add(new RedisNode(parts[0],
Integer.parseInt(parts[1])));
+ redisNodes.add(parseRedisNode(node));
}
return redisNodes;
}
+
+ private RedisNode parseRedisNode(final String url) {
+ String host = url;
+ int port = 6379;
+ int lastColonIndex = url.lastIndexOf(":");
+ int bracketIndex = url.lastIndexOf("]");
+ if (bracketIndex > -1) {
+ if (lastColonIndex > bracketIndex) {
+ host = url.substring(0, lastColonIndex);
+ port = Integer.parseInt(url.substring(lastColonIndex + 1));
+ }
+ } else {
+ if (lastColonIndex > -1) {
+ host = url.substring(0, lastColonIndex);
+ port = Integer.parseInt(url.substring(lastColonIndex + 1));
+ }
+ }
+ return new RedisNode(host, port);
+ }
Review Comment:
The new `parseRedisNode` method lacks test coverage. Consider adding tests
for various scenarios including: IPv4 addresses with ports (e.g.,
"localhost:6379"), IPv6 addresses with ports (e.g., "[::1]:6379"), IPv6
addresses without ports (e.g., "[::1]"), addresses without ports using the
default 6379, and error cases like malformed URLs or invalid port numbers.
##########
shenyu-infra/shenyu-infra-redis/src/main/java/org/apache/shenyu/infra/redis/RedisConnectionFactory.java:
##########
@@ -126,10 +135,27 @@ private List<RedisNode> createRedisNode(final String url)
{
List<RedisNode> redisNodes = new ArrayList<>();
List<String> nodes = Lists.newArrayList(Splitter.on(";").split(url));
for (String node : nodes) {
- String[] parts = StringUtils.split(node, ":");
- Assert.state(Objects.requireNonNull(parts).length == 2, "Must be
defined as 'host:port'");
- redisNodes.add(new RedisNode(parts[0],
Integer.parseInt(parts[1])));
+ redisNodes.add(parseRedisNode(node));
}
return redisNodes;
}
+
+ private RedisNode parseRedisNode(final String url) {
+ String host = url;
+ int port = 6379;
+ int lastColonIndex = url.lastIndexOf(":");
+ int bracketIndex = url.lastIndexOf("]");
+ if (bracketIndex > -1) {
+ if (lastColonIndex > bracketIndex) {
+ host = url.substring(0, lastColonIndex);
+ port = Integer.parseInt(url.substring(lastColonIndex + 1));
+ }
+ } else {
+ if (lastColonIndex > -1) {
+ host = url.substring(0, lastColonIndex);
+ port = Integer.parseInt(url.substring(lastColonIndex + 1));
Review Comment:
Potential uncaught 'java.lang.NumberFormatException'.
```suggestion
try {
port = Integer.parseInt(url.substring(lastColonIndex +
1));
} catch (NumberFormatException e) {
port = 6379; // fallback to default port
}
}
} else {
if (lastColonIndex > -1) {
host = url.substring(0, lastColonIndex);
try {
port = Integer.parseInt(url.substring(lastColonIndex +
1));
} catch (NumberFormatException e) {
port = 6379; // fallback to default port
}
```
##########
shenyu-infra/shenyu-infra-etcd/src/main/java/org/apache/shenyu/infra/etcd/client/EtcdClient.java:
##########
@@ -375,22 +363,58 @@ public GetResponse getRange(final String key, final
GetOption getOption) {
/**
* put data as ephemeral.
- * @param key key
+ *
+ * @param key key
* @param value value
*/
public void putEphemeral(final String key, final String value) {
try {
KV kvClient = client.getKVClient();
kvClient.put(ByteSequence.from(key, UTF_8),
ByteSequence.from(value, UTF_8),
-
PutOption.newBuilder().withLeaseId(globalLeaseId).build())
+ PutOption.newBuilder().withLeaseId(globalLeaseId).build())
.get(timeout, TimeUnit.MILLISECONDS);
} catch (InterruptedException | ExecutionException | TimeoutException
e) {
LOG.error("putEphemeral(key:{},value:{}) error.", key, value, e);
+ throw new ShenyuException(e);
}
}
+ private void initLease() {
+ try {
+ this.globalLeaseId =
client.getLeaseClient().grant(ttl).get().getID();
+ keepAlive();
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.error("initLease error.", e);
+ throw new ShenyuException(e);
+ }
+ }
+
+ private void keepAlive() {
+ client.getLeaseClient().keepAlive(globalLeaseId, new
StreamObserver<>() {
+ @Override
+ public void onNext(final LeaseKeepAliveResponse
leaseKeepAliveResponse) {
+ }
+
+ @Override
+ public void onError(final Throwable throwable) {
+ LOG.error("keep alive error", throwable);
+ try {
+ TimeUnit.SECONDS.sleep(1);
+ } catch (InterruptedException e) {
+ LOG.error("keep alive sleep error", e);
+ }
+ keepAlive();
+ }
+
+ @Override
+ public void onCompleted() {
+ }
+ });
+ }
Review Comment:
The refactored `keepAlive` method and its recursive error handling logic
lack test coverage. The existing test in `EtcdClientTest` only covers the basic
`onError` callback but doesn't verify the new retry behavior with sleep and
recursive call. Consider adding tests to verify the error recovery logic.
--
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]
