Copilot commented on code in PR #3315:
URL: https://github.com/apache/hertzbeat/pull/3315#discussion_r2072496964
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/util/AlertTemplateUtil.java:
##########
@@ -38,20 +39,22 @@ private AlertTemplateUtil() {
private static final Pattern PATTERN = Pattern.compile("\\$\\{(\\w+)\\}");
public static String render(String template, Map<String, Object>
replaceData) {
- if (template == null) {
- return null;
+ if (!StringUtils.hasText(template)) {
+ return template;
}
if (replaceData == null) {
- log.warn("The replaceData is null.");
+ log.warn("The render replace data is null.");
return template;
}
try {
Matcher matcher = PATTERN.matcher(template);
StringBuilder builder = new StringBuilder();
while (matcher.find()) {
Object objectValue =
replaceData.getOrDefault(matcher.group(1), "NullValue");
- String value = objectValue.toString();
- matcher.appendReplacement(builder,
Matcher.quoteReplacement(value));
+ if (objectValue != null) {
+ String value = objectValue.toString();
+ matcher.appendReplacement(builder,
Matcher.quoteReplacement(value));
+ }
Review Comment:
If 'objectValue' is null, the token remains unreplaced; please confirm if
skipping replacement in such cases is the intended behavior.
```suggestion
String value = objectValue != null ? objectValue.toString()
: "NullValue";
matcher.appendReplacement(builder,
Matcher.quoteReplacement(value));
```
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/DbAlertStoreHandlerImpl.java:
##########
@@ -52,86 +52,84 @@ public GroupAlert store(GroupAlert groupAlert) {
log.error("The Group Alerts is empty, ignore store");
return groupAlert;
}
- // 1. Find existing alert group
- GroupAlert existGroupAlert =
groupAlertDao.findByGroupKey(groupAlert.getGroupKey());
-
- // 2. Process individual alerts
+ // Process individual alerts
Set<String> alertFingerprints = new HashSet<>(8);
-
List<SingleAlert> originalAlerts = groupAlert.getAlerts();
List<SingleAlert> newAlerts = new ArrayList<>();
-
for (SingleAlert singleAlert : originalAlerts) {
- SingleAlert existAlert =
singleAlertDao.findByFingerprint(singleAlert.getFingerprint());
-
- if (existAlert != null) {
- // Update the existing alert with the ID and creation time
from the database
- singleAlert.setId(existAlert.getId());
- singleAlert.setGmtCreate(existAlert.getGmtCreate());
-
- // Status transition logic
- if
(CommonConstants.ALERT_STATUS_FIRING.equals(singleAlert.getStatus())) {
- // If the alert is firing and the existing alert is not
resolved, update the start time and trigger times
- if
(!CommonConstants.ALERT_STATUS_RESOLVED.equals(existAlert.getStatus())) {
+ synchronized (singleAlert.getFingerprint().intern()) {
Review Comment:
[nitpick] Synchronizing on string.intern() may lead to unintended contention
if the fingerprints are not strictly controlled; consider using a dedicated
lock object for improved clarity and safety.
```suggestion
Object lock =
lockRegistry.computeIfAbsent(singleAlert.getFingerprint(), key -> new Object());
synchronized (lock) {
```
##########
hertzbeat-common/src/main/java/org/apache/hertzbeat/common/entity/job/Job.java:
##########
@@ -114,7 +114,7 @@ public class Job {
/**
* Refresh time list for one cycle of the job
*/
- private LinkedList<Long> intervals;
+ private volatile LinkedList<Long> intervals;
Review Comment:
[nitpick] Using 'volatile' for a LinkedList does not guarantee thread-safety
for compound operations; consider using a thread-safe collection like
ConcurrentLinkedQueue or ensure that all accesses are properly synchronized.
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/DbAlertStoreHandlerImpl.java:
##########
@@ -52,86 +52,84 @@ public GroupAlert store(GroupAlert groupAlert) {
log.error("The Group Alerts is empty, ignore store");
return groupAlert;
}
- // 1. Find existing alert group
- GroupAlert existGroupAlert =
groupAlertDao.findByGroupKey(groupAlert.getGroupKey());
-
- // 2. Process individual alerts
+ // Process individual alerts
Set<String> alertFingerprints = new HashSet<>(8);
-
List<SingleAlert> originalAlerts = groupAlert.getAlerts();
List<SingleAlert> newAlerts = new ArrayList<>();
-
for (SingleAlert singleAlert : originalAlerts) {
- SingleAlert existAlert =
singleAlertDao.findByFingerprint(singleAlert.getFingerprint());
-
- if (existAlert != null) {
- // Update the existing alert with the ID and creation time
from the database
- singleAlert.setId(existAlert.getId());
- singleAlert.setGmtCreate(existAlert.getGmtCreate());
-
- // Status transition logic
- if
(CommonConstants.ALERT_STATUS_FIRING.equals(singleAlert.getStatus())) {
- // If the alert is firing and the existing alert is not
resolved, update the start time and trigger times
- if
(!CommonConstants.ALERT_STATUS_RESOLVED.equals(existAlert.getStatus())) {
+ synchronized (singleAlert.getFingerprint().intern()) {
+ SingleAlert existAlert =
singleAlertDao.findByFingerprint(singleAlert.getFingerprint());
+ if (existAlert != null) {
+ // Update the existing alert with the ID and creation time
from the database
+ singleAlert.setId(existAlert.getId());
+ singleAlert.setGmtCreate(existAlert.getGmtCreate());
+ // Status transition logic
+ if
(CommonConstants.ALERT_STATUS_FIRING.equals(singleAlert.getStatus())) {
+ // If the alert is firing and the existing alert is
not resolved, update the start time and trigger times
+ if
(!CommonConstants.ALERT_STATUS_RESOLVED.equals(existAlert.getStatus())) {
+ singleAlert.setStartAt(existAlert.getStartAt());
+ int triggerTimes =
Optional.ofNullable(existAlert.getTriggerTimes()).orElse(1)
+ +
Optional.ofNullable(singleAlert.getTriggerTimes()).orElse(1);
+ singleAlert.setTriggerTimes(triggerTimes);
+ }
+ } else if
(CommonConstants.ALERT_STATUS_RESOLVED.equals(singleAlert.getStatus())) {
+ // If the alert is resolved, set the end time (if not
already set) and copy other fields from the existing alert
+ if (singleAlert.getEndAt() == null) {
+ singleAlert.setEndAt(System.currentTimeMillis());
+ }
singleAlert.setStartAt(existAlert.getStartAt());
- int triggerTimes =
Optional.ofNullable(existAlert.getTriggerTimes()).orElse(1)
- +
Optional.ofNullable(singleAlert.getTriggerTimes()).orElse(1);
- singleAlert.setTriggerTimes(triggerTimes);
- }
- } else if
(CommonConstants.ALERT_STATUS_RESOLVED.equals(singleAlert.getStatus())) {
- // If the alert is resolved, set the end time (if not
already set) and copy other fields from the existing alert
- if (singleAlert.getEndAt() == null) {
- singleAlert.setEndAt(System.currentTimeMillis());
+ singleAlert.setActiveAt(existAlert.getActiveAt());
+
singleAlert.setTriggerTimes(existAlert.getTriggerTimes());
}
- singleAlert.setStartAt(existAlert.getStartAt());
- singleAlert.setActiveAt(existAlert.getActiveAt());
- singleAlert.setTriggerTimes(existAlert.getTriggerTimes());
}
+ SingleAlert savedSingleAlert =
singleAlertDao.save(singleAlert);
+ newAlerts.add(savedSingleAlert);
+ alertFingerprints.add(savedSingleAlert.getFingerprint());
}
- SingleAlert savedSingleAlert = singleAlertDao.save(singleAlert);
- newAlerts.add(savedSingleAlert);
- alertFingerprints.add(savedSingleAlert.getFingerprint());
}
groupAlert.setAlerts(newAlerts);
- // 3. Process resolved alerts
- if (existGroupAlert != null) {
- List<String> existFingerprints =
existGroupAlert.getAlertFingerprints();
- if (existFingerprints != null) {
- alertFingerprints.addAll(existFingerprints);
- }
- // Merge group information
- groupAlert.setId(existGroupAlert.getId());
- groupAlert.setGmtCreate(existGroupAlert.getGmtCreate());
- // Merge other historical information to preserve
- Map<String, String> existCommonLabels =
existGroupAlert.getCommonLabels();
- if (existCommonLabels != null) {
- Map<String, String> commonLabels =
groupAlert.getCommonLabels();
- if (commonLabels != null) {
- // filter common label in commonLabels and
existCommonLabels
- commonLabels = commonLabels.entrySet().stream()
- .filter(entry ->
existCommonLabels.containsKey(entry.getKey()))
- .collect(Collectors.toMap(Map.Entry::getKey,
Map.Entry::getValue));
- groupAlert.setCommonLabels(commonLabels);
+ // Find existing alert group
+ synchronized (groupAlert.getGroupKey().intern()) {
Review Comment:
[nitpick] Using string.intern() for locking on group keys may result in
unexpected contention; consider using a dedicated lock per group if high
concurrency is expected.
```suggestion
Object lock =
groupKeyLocks.computeIfAbsent(groupAlert.getGroupKey(), key -> new Object());
synchronized (lock) {
```
--
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: notifications-unsubscr...@hertzbeat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@hertzbeat.apache.org
For additional commands, e-mail: notifications-h...@hertzbeat.apache.org
