Copilot commented on code in PR #4099:
URL: https://github.com/apache/hertzbeat/pull/4099#discussion_r3005652718
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##########
@@ -97,7 +111,17 @@ public List<ServiceInstance> getServices() {
}
List<ServiceInstance> serviceInstanceList = Lists.newArrayList();
try {
- for (String serviceName : namingService.getServicesOfServer(0,
9999).getData()) {
+ 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;
+ }
namingService.getAllInstances(serviceName).forEach(instance ->
Review Comment:
When `groupName` is specified, service names are fetched from that group,
but instance lookup still uses `namingService.getAllInstances(serviceName)`
(default group). This will return instances from the wrong group (or none).
Pass `localConnectConfig.getGroupName()` to the instance query as well (and
ensure the default-group behavior remains unchanged when `groupName` is blank).
```suggestion
List<com.alibaba.nacos.api.naming.pojo.Instance> instances;
if (StringUtils.hasText(localConnectConfig.getGroupName())) {
instances = namingService.getAllInstances(serviceName,
localConnectConfig.getGroupName());
} else {
instances = namingService.getAllInstances(serviceName);
}
instances.forEach(instance ->
```
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##########
@@ -55,7 +63,13 @@ public void initClient(ConnectConfig connectConfig) {
try {
localConnectConfig = connectConfig;
- namingService =
NamingFactory.createNamingService(connectConfig.getHost() + ":" +
connectConfig.getPort());
+ Properties properties = new Properties();
+ properties.put("serverAddr", connectConfig.getHost() + ":" +
connectConfig.getPort());
+ properties.put("username", connectConfig.getUsername());
+ properties.put("password", connectConfig.getPassword());
+ properties.put("namespace", connectConfig.getNamespace());
Review Comment:
`Properties` (which extends `Hashtable`) does not allow null values. Since
`username`/`password`/`namespace` are optional, `properties.put("username",
connectConfig.getUsername())` etc. will throw `NullPointerException` when any
of these are unset. Only set these properties when they are non-empty (e.g.,
guard with `StringUtils.hasText(...)`) or use a helper that skips null/blank
values.
```suggestion
if (StringUtils.hasText(connectConfig.getUsername())) {
properties.put("username", connectConfig.getUsername());
}
if (StringUtils.hasText(connectConfig.getPassword())) {
properties.put("password", connectConfig.getPassword());
}
if (StringUtils.hasText(connectConfig.getNamespace())) {
properties.put("namespace", connectConfig.getNamespace());
}
```
##########
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/registry/discovery/impl/NacosDiscoveryClient.java:
##########
@@ -55,7 +63,13 @@ public void initClient(ConnectConfig connectConfig) {
try {
localConnectConfig = connectConfig;
- namingService =
NamingFactory.createNamingService(connectConfig.getHost() + ":" +
connectConfig.getPort());
+ Properties properties = new Properties();
+ properties.put("serverAddr", connectConfig.getHost() + ":" +
connectConfig.getPort());
+ properties.put("username", connectConfig.getUsername());
+ properties.put("password", connectConfig.getPassword());
+ properties.put("namespace", connectConfig.getNamespace());
+
+ namingService = NamingFactory.createNamingService(properties);
Review Comment:
`initClient` now calls `NamingFactory.createNamingService(Properties)`, but
the existing unit test `NacosDiscoveryClientTest#testInitClientSuccess`
mocks/verify the `createNamingService(String)` overload. This change will break
that test (and it currently doesn't assert that username/password/namespace are
passed). Update the tests to mock/verify the `Properties` overload and add
coverage for the new auth/namespace initialization and groupName/serviceName
filtering behavior.
##########
hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/NacosSdProtocol.java:
##########
@@ -52,7 +52,18 @@ public class NacosSdProtocol implements Protocol {
* Nacos password for authentication
*/
private String password;
-
+
+ /**
+ * Nacos Service Name;
+ */
+ private String serviceName;
+
+ /**
+ * Nacos Group Name;
+ */
+
Review Comment:
Javadoc punctuation/formatting: remove the trailing semicolons in "Nacos
Service Name;" / "Nacos Group Name;" and the extra blank line before
`groupName` to match typical Javadoc style used elsewhere in the codebase.
```suggestion
* Nacos Service Name
*/
private String serviceName;
/**
* Nacos Group Name
*/
```
##########
hertzbeat-manager/src/main/resources/define/app-nacos_sd.yml:
##########
@@ -42,6 +42,42 @@ 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 Name Space
Review Comment:
English label uses "Name Space"; this is typically spelled as the single
word "Namespace" and is inconsistent with other labels in the same file (e.g.,
"Service Name", "Group Name"). Consider changing to "Nacos Service Discovery
Namespace" for consistency.
```suggestion
en-US: Nacos Service Discovery Namespace
```
--
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]
