Copilot commented on code in PR #6082:
URL: https://github.com/apache/shenyu/pull/6082#discussion_r2249883429
##########
shenyu-infra/shenyu-infra-nacos/src/main/java/org/apache/shenyu/infra/nacos/config/NacosConfig.java:
##########
@@ -19,7 +19,7 @@
import java.util.Objects;
-public class NacosConfig {
+public final class NacosConfig {
Review Comment:
Making NacosConfig a final class breaks backward compatibility. The original
class was not final, so existing code that extends this class will fail to
compile. Consider removing the final modifier to maintain API compatibility.
```suggestion
public class NacosConfig {
```
##########
shenyu-infra/shenyu-infra-nacos/src/main/java/org/apache/shenyu/infra/nacos/config/NacosACMConfig.java:
##########
@@ -19,7 +19,7 @@
import java.util.Objects;
-public class NacosACMConfig {
+public final class NacosACMConfig {
Review Comment:
Making NacosACMConfig a final class breaks backward compatibility. The
original class was not final, so existing code that extends this class will
fail to compile. Consider removing the final modifier to maintain API
compatibility.
```suggestion
public class NacosACMConfig {
```
##########
shenyu-infra/shenyu-infra-redis/pom.xml:
##########
@@ -17,11 +17,35 @@
-->
<project xmlns="http://maven.apache.org/POM/4.0.0";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+
<parent>
<groupId>org.apache.shenyu</groupId>
<artifactId>shenyu-infra</artifactId>
<version>2.7.0.2-SNAPSHOT</version>
</parent>
+
<modelVersion>4.0.0</modelVersion>
<artifactId>shenyu-infra-redis</artifactId>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.shenyu</groupId>
+ <artifactId>shenyu-plugin-ai-common</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
Review Comment:
The shenyu-infra-redis module should not depend on shenyu-plugin-ai-common.
Infrastructure modules should be independent and not depend on specific plugin
modules to maintain proper separation of concerns.
```suggestion
```
##########
shenyu-infra/shenyu-infra-etcd/src/main/java/org/apache/shenyu/infra/etcd/config/EtcdConfig.java:
##########
@@ -22,7 +22,7 @@
/**
* The type etcd configuration.
*/
-public class EtcdConfig {
+public final class EtcdConfig {
Review Comment:
Making EtcdConfig a final class breaks backward compatibility. The original
class was not final, so existing code that extends this class will fail to
compile. Consider removing the final modifier to maintain API compatibility.
```suggestion
public class EtcdConfig {
```
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-sync-data-center/shenyu-spring-boot-starter-sync-data-nacos/src/main/java/org/apache/shenyu/springboot/starter/sync/data/nacos/NacosSyncDataConfiguration.java:
##########
@@ -117,8 +117,9 @@ public ConfigService nacosConfigService(final NacosConfig
nacosConfig) throws Ex
* @return the http config
*/
@Bean
- @ConfigurationProperties(prefix = "shenyu.sync.nacos")
+ @ConditionOnSyncNacos
Review Comment:
The @ConfigurationProperties annotation has been removed from the
nacosConfig() method without replacement. This means the configuration
properties will no longer be automatically bound from application properties,
potentially breaking configuration loading.
```suggestion
@ConditionOnSyncNacos
@ConfigurationProperties(prefix = "shenyu.sync.nacos")
```
##########
shenyu-infra/shenyu-infra-etcd/src/main/java/org/apache/shenyu/infra/etcd/client/EtcdClient.java:
##########
@@ -46,19 +55,27 @@
*/
public class EtcdClient {
- /**
- * logger.
- */
private static final Logger LOG =
LoggerFactory.getLogger(EtcdClient.class);
private final Client client;
+ private final long ttl;
+
+ private final long timeout;
+
+ private long globalLeaseId;
+
private final ConcurrentHashMap<String, Watch.Watcher> watchCache = new
ConcurrentHashMap<>();
private final ConcurrentHashMap<String, Watch.Watcher> watchChildCache =
new ConcurrentHashMap<>();
- public EtcdClient(final Client client) {
+ public EtcdClient(final Client client, final long ttl, final long timeout)
{
+
+ this.ttl = ttl;
+ this.timeout = timeout;
this.client = client;
+
Review Comment:
The EtcdClient constructor signature has changed from accepting only a
Client to requiring ttl and timeout parameters. This is a breaking change that
will affect existing code. Consider providing backward-compatible constructors
or factory methods.
```suggestion
/**
* Backward-compatible constructor for legacy code.
* Uses default ttl and timeout values.
*
* @param client etcd client
*/
public EtcdClient(final Client client) {
this(client, 60L, 5L); // default ttl=60, timeout=5
}
public EtcdClient(final Client client, final long ttl, final long
timeout) {
this.ttl = ttl;
this.timeout = timeout;
this.client = client;
```
--
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]
