Copilot commented on code in PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099#discussion_r3005880081
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##########
@@ -97,8 +119,25 @@ public List<ServiceInstance> getServices() {
}
List<ServiceInstance> serviceInstanceList = Lists.newArrayList();
try {
- for (String serviceName : namingService.getServicesOfServer(0,
9999).getData()) {
- namingService.getAllInstances(serviceName).forEach(instance ->
+ List<String> services ;
+ if(StringUtils.hasText(localConnectConfig.getGroupName())) {
+ services = namingService.getServicesOfServer(0, 9999,
localConnectConfig.getGroupName()).getData();
+ } else {
+ services = namingService.getServicesOfServer(0, 9999).getData();
+ }
+
+ for (String serviceName : services) {
+
if(StringUtils.hasText(localConnectConfig.getServiceName())&&!serviceName.equals(localConnectConfig.getServiceName())){
+ continue;
+ }
+ List<Instance> instances;
+ if(StringUtils.hasText(localConnectConfig.getGroupName())){
+ instances = namingService.getAllInstances(serviceName,
localConnectConfig.getGroupName());
+ }else{
+ instances = namingService.getAllInstances(serviceName);
+ }
Review Comment:
New groupName/serviceName branches are not covered by unit tests (e.g.,
using getServicesOfServer(page,size,groupName),
getAllInstances(service,groupName), and verifying serviceName filtering). Add
test cases for: (1) groupName set, (2) serviceName set, and (3) both set,
asserting the correct NamingService methods are invoked and results are
filtered as expected.
##########
hertzbeat-manager/src/main/resources/define/app-nacos_sd.yml:
##########
@@ -42,6 +42,78 @@ params:
ja-JP: ポート
type: number
required: true
+ - field: __nacos_sd_username__
+ name:
+ zh-CN: Nacos 服务发现 Username
+ en-US: Nacos Service Discovery User Name
+ ja-JP: ユーザー名
+ type: text
+ required: false
+ - field: __nacos_sd_password__
+ name:
+ zh-CN: Nacos 服务发现 Password
+ en-US: Nacos Service Discovery Password
+ ja-JP: パスワード
+ type: password
+ required: false
+ - field: __nacos_sd_namespace__
+ name:
+ zh-CN: Nacos 服务发现 Namespace
+ en-US: Nacos Service Discovery Namespace
+ ja-JP: 名前空間
+ type: text
+ required: false
+ - field: __nacos_sd_groupName__
+ name:
+ zh-CN: Nacos 服务发现 Group Name
+ en-US: Nacos Service Discovery Group Name
+ ja-JP: グループ名
+ type: text
+ required: false
+ - field: __nacos_sd_serviceName__
+ name:
+ zh-CN: Nacos 服务发现 Service Name
+ en-US: Nacos Service Discovery Service Name
+ ja-JP: サービス名
+ type: text
+ required: false
+
+ - field: __nacos_sd_username__
+ name:
+ zh-CN: Nacos 服务发现 Username
+ en-US: Nacos Service Discovery User Name
+ ja-JP: ユーザー名
+ type: text
+ required: false
+ - field: __nacos_sd_password__
+ name:
+ zh-CN: Nacos 服务发现 Password
+ en-US: Nacos Service Discovery Password
+ ja-JP: パスワード
+ type: password
+ required: false
+ - field: __nacos_sd_namespace__
+ name:
+ zh-CN: Nacos 服务发现 Namespace
+ en-US: Nacos Service Discovery Namespace
+ ja-JP: 名前空間
+ type: text
+ required: false
+ - field: __nacos_sd_groupName__
+ name:
+ zh-CN: Nacos 服务发现 Group Name
+ en-US: Nacos Service Discovery Group Name
+ ja-JP: グループ名
+ type: text
+ required: false
+ - field: __nacos_sd_serviceName__
+ name:
+ zh-CN: Nacos 服务发现 Service Name
+ en-US: Nacos Service Discovery Service Name
+ ja-JP: サービス名
+ type: text
+ required: false
+
Review Comment:
The params section defines the same five fields twice
(__nacos_sd_username__/password/namespace/groupName/serviceName). This will
likely render duplicate inputs and can cause ambiguous parameter resolution.
Remove the duplicated second block so each field is defined only once.
```suggestion
```
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##########
@@ -97,8 +119,25 @@ public List<ServiceInstance> getServices() {
}
List<ServiceInstance> serviceInstanceList = Lists.newArrayList();
try {
- for (String serviceName : namingService.getServicesOfServer(0,
9999).getData()) {
- namingService.getAllInstances(serviceName).forEach(instance ->
+ List<String> services ;
+ if(StringUtils.hasText(localConnectConfig.getGroupName())) {
+ services = namingService.getServicesOfServer(0, 9999,
localConnectConfig.getGroupName()).getData();
+ } else {
+ services = namingService.getServicesOfServer(0, 9999).getData();
+ }
+
+ for (String serviceName : services) {
+
if(StringUtils.hasText(localConnectConfig.getServiceName())&&!serviceName.equals(localConnectConfig.getServiceName())){
+ continue;
+ }
+ List<Instance> instances;
+ if(StringUtils.hasText(localConnectConfig.getGroupName())){
+ instances = namingService.getAllInstances(serviceName,
localConnectConfig.getGroupName());
+ }else{
Review Comment:
There are several formatting issues in the new block (e.g., `List<String>
services ;`, missing spaces after `if`, missing spaces around `&&`, and
inconsistent brace/indent spacing). Please format this section consistently
with the rest of the class to improve readability and reduce diffs in future
changes.
```suggestion
List<String> services;
if (StringUtils.hasText(localConnectConfig.getGroupName())) {
services = namingService.getServicesOfServer(0, 9999,
localConnectConfig.getGroupName()).getData();
} else {
services = namingService.getServicesOfServer(0,
9999).getData();
}
for (String serviceName : services) {
if (StringUtils.hasText(localConnectConfig.getServiceName())
&&
!serviceName.equals(localConnectConfig.getServiceName())) {
continue;
}
List<Instance> instances;
if (StringUtils.hasText(localConnectConfig.getGroupName())) {
instances = namingService.getAllInstances(serviceName,
localConnectConfig.getGroupName());
} else {
```
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##########
@@ -97,8 +119,25 @@ public List<ServiceInstance> getServices() {
}
List<ServiceInstance> serviceInstanceList = Lists.newArrayList();
try {
- for (String serviceName : namingService.getServicesOfServer(0,
9999).getData()) {
- namingService.getAllInstances(serviceName).forEach(instance ->
+ List<String> services ;
+ if(StringUtils.hasText(localConnectConfig.getGroupName())) {
+ services = namingService.getServicesOfServer(0, 9999,
localConnectConfig.getGroupName()).getData();
+ } else {
+ services = namingService.getServicesOfServer(0, 9999).getData();
+ }
+
+ for (String serviceName : services) {
+
if(StringUtils.hasText(localConnectConfig.getServiceName())&&!serviceName.equals(localConnectConfig.getServiceName())){
+ continue;
+ }
Review Comment:
When serviceName is provided, the implementation still fetches the full
service list (page size 9999) and then filters client-side. This adds
unnecessary Nacos calls and can become expensive in large registries. Consider
short-circuiting: if serviceName is set, directly call
getAllInstances(serviceName[, groupName]) without calling getServicesOfServer().
--
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]
