Copilot commented on code in PR #4101:
URL: https://github.com/apache/hertzbeat/pull/4101#discussion_r3006296805
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/MonitorServiceImpl.java:
##########
@@ -396,8 +402,6 @@ public void modifyMonitor(Monitor monitor, List<Param>
params, String collector,
}
monitor.setInstance(instance);
Review Comment:
`modifyMonitor` appends `portWithMark` whenever `instance` is non-null, even
if `instance` already includes a ":port" suffix. This can produce duplicated
ports (e.g., `host:9200:9200`) for API clients/imports that send
`monitor.instance` with port included. Align this logic with `addMonitor` by
checking `IpDomainUtil.isHasPortWithMark(instance)` before concatenating the
port.
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/MonitorServiceImpl.java:
##########
@@ -382,6 +382,12 @@ public void modifyMonitor(Monitor monitor, List<Param>
params, String collector,
labelDao.saveAll(addLabels);
}
+ boolean isStatic =
CommonConstants.SCRAPE_STATIC.equals(monitor.getScrape())
+ || !StringUtils.hasText(monitor.getScrape());
+ if (!isStatic) {
+ monitor.setInstance("unknow");
Review Comment:
The placeholder instance string is spelled "unknow". If this value is
user-facing or persisted, consider switching to the correctly spelled "unknown"
(ideally via a shared constant) and handling any
backward-compatibility/migration needed, since this method now introduces an
additional persisted usage of the typo.
```suggestion
monitor.setInstance("unknown");
```
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/MonitorServiceImpl.java:
##########
@@ -382,6 +382,12 @@ public void modifyMonitor(Monitor monitor, List<Param>
params, String collector,
labelDao.saveAll(addLabels);
}
+ boolean isStatic =
CommonConstants.SCRAPE_STATIC.equals(monitor.getScrape())
+ || !StringUtils.hasText(monitor.getScrape());
+ if (!isStatic) {
Review Comment:
This change fixes a service-discovery modification path where
`monitor.instance` could be null and later used in `Map.of(...)` (which rejects
null values). There is no unit test covering `modifyMonitor` with a non-static
`scrape` and a null/blank `instance`; adding one would prevent regressions and
validate the issue #4100 scenario.
```suggestion
if (!isStatic && !StringUtils.hasText(monitor.getInstance())) {
```
--
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]
