Copilot commented on code in PR #4168:
URL: https://github.com/apache/hertzbeat/pull/4168#discussion_r3522709352
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/reduce/AlarmGroupReduce.java:
##########
@@ -294,13 +294,17 @@ private void sendGroupAlert(GroupAlertCache cache) {
AlertGroupConverge ruleConfig =
groupDefines.get(cache.getGroupDefineName());
long repeatInterval = ruleConfig.getRepeatInterval() != null
? ruleConfig.getRepeatInterval() * MS_PER_SECOND :
DEFAULT_REPEAT_INTERVAL;
Review Comment:
`sendGroupAlert` assumes `groupDefines` still contains
`cache.getGroupDefineName()`. If group rules are refreshed/removed while caches
exist, `ruleConfig` can be null and `ruleConfig.getRepeatInterval()` will throw
NPE. `shouldSendGroup(...)` already handles `ruleConfig == null`, so
`sendGroupAlert(...)` should too and fall back to defaults.
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/DbAlertStoreHandlerImpl.java:
##########
@@ -127,9 +127,26 @@ public GroupAlert store(GroupAlert groupAlert) {
}
// Save alert group
groupAlert.setAlertFingerprints(alertFingerprints.stream().toList());
+ refreshGroupStatus(groupAlert);
GroupAlert savedGroupAlert = groupAlertDao.save(groupAlert);
savedGroupAlert.setAlerts(groupAlert.getAlerts());
return savedGroupAlert;
}
}
+
+ private void refreshGroupStatus(GroupAlert groupAlert) {
+ List<String> alertFingerprints = groupAlert.getAlertFingerprints();
+ if (alertFingerprints == null || alertFingerprints.isEmpty()) {
+ return;
+ }
+ List<SingleAlert> alerts =
singleAlertDao.findSingleAlertsByFingerprintIn(alertFingerprints);
+ if (alerts == null) {
+ return;
+ }
+ boolean hasFiringAlert = alerts.stream()
+ .anyMatch(alert ->
CommonConstants.ALERT_STATUS_FIRING.equals(alert.getStatus()));
Review Comment:
`refreshGroupStatus` can change the group `status` based on alerts loaded
from the DB, but the outgoing `GroupAlert` still carries only the transient
`alerts` list from the current push. This can produce inconsistent payloads
(e.g., `status=firing` while `alerts` contains only a resolved member), and
templates/plugins rely on `alert.getAlerts()` for content.
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/DbAlertStoreHandlerImpl.java:
##########
@@ -127,9 +127,26 @@ public GroupAlert store(GroupAlert groupAlert) {
}
// Save alert group
groupAlert.setAlertFingerprints(alertFingerprints.stream().toList());
+ refreshGroupStatus(groupAlert);
GroupAlert savedGroupAlert = groupAlertDao.save(groupAlert);
Review Comment:
`refreshGroupStatus(groupAlert)` performs a DB lookup over the full
`alertFingerprints` set on every store call. Since this is only needed to
correct the *resolved* misclassification case (when the incoming payload says
`resolved` but historical members are still firing), you can skip the DB query
when the incoming group is already `firing` to reduce load.
--
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]
