feat: Add dedicated thread pool for RestTemplate in alert notificati…#3481
Open
Carpe-Wang wants to merge 26 commits intoapache:masterfrom
Open
feat: Add dedicated thread pool for RestTemplate in alert notificati…#3481Carpe-Wang wants to merge 26 commits intoapache:masterfrom
Carpe-Wang wants to merge 26 commits intoapache:masterfrom
Conversation
…into RestTemplateConfig-Thread-pool
tomsun28
reviewed
Jun 21, 2025
Comment on lines
66
to
68
| executor.setCorePoolSize(10); | ||
| executor.setMaxPoolSize(50); | ||
| executor.setQueueCapacity(200); |
Member
There was a problem hiding this comment.
hi the http request require timeliness, suggest use the SynchronousQueue instead of executor.setQueueCapacity(200); , and executor.setCorePoolSize(2); executor.setMaxPoolSize(Integer.MAX_VALUE);
tomsun28
reviewed
Jun 21, 2025
Comment on lines
122
to
133
| protected CompletableFuture<Void> sendAsync(org.apache.hertzbeat.common.entity.alerter.NoticeReceiver receiver, | ||
| org.apache.hertzbeat.common.entity.alerter.NoticeTemplate noticeTemplate, | ||
| GroupAlert alert) { | ||
| return CompletableFuture.runAsync(() -> { | ||
| try { | ||
| send(receiver, noticeTemplate, alert); | ||
| } catch (Exception e) { | ||
| log.error("Async alert notification failed", e); | ||
| throw new RuntimeException(e); | ||
| } | ||
| }, restTemplateThreadPool); | ||
| } |
Member
There was a problem hiding this comment.
seem no others use this method.
Contributor
Author
There was a problem hiding this comment.
I have deleted this piece of code.
tomsun28
reviewed
Jun 21, 2025
Comment on lines
138
to
146
| CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])) | ||
| .whenComplete((result, exception) -> { | ||
| if (exception != null) { | ||
| log.warn("Some async notifications failed", exception); | ||
| } else { | ||
| log.debug("All notifications completed for alert: {}", alert.getGroupLabels()); | ||
| } | ||
| }); | ||
| }))); |
Member
There was a problem hiding this comment.
hi can you describe why use the CompleableFuture here, sendNoticeMsg does not need to return a result; it just needs to handle exceptions.
Member
|
hi suggest merge this in 1.7.3 after 1.7.2 released. |
tomsun28
reviewed
Sep 19, 2025
Comment on lines
+131
to
+137
| matchNoticeRulesByAlert(alert).ifPresent(noticeRules -> noticeRules.forEach(rule -> workerPool.executeNotify(() -> { | ||
| rule.getReceiverId().forEach(receiverId -> { | ||
| restTemplateThreadPool.execute(() -> { | ||
| try { | ||
| sendNoticeMsg(getOneReceiverById(receiverId), | ||
| getOneTemplateById(rule.getTemplateId()), alert); | ||
| } catch (AlertNoticeException e) { | ||
| log.warn("DispatchTask sendNoticeMsg error, message: {}", e.getMessage()); | ||
| sendNoticeMsg(getOneReceiverById(receiverId), getOneTemplateById(rule.getTemplateId()), alert); | ||
| } catch (Exception e) { | ||
| log.warn("Async notification failed for receiver {}: {}", receiverId, e.getMessage()); |
Member
There was a problem hiding this comment.
Looking back at this PR again, it is not recommended that the alterer module use the manager module's bean, and the notification uses two thread pools. suggest that use the workerPool instead of restTemplateThreadPool directly.
matchNoticeRulesByAlert(alert).ifPresent(noticeRules -> noticeRules.forEach(rule -> rule.getReceiverId()
.forEach(receiverId -> {
workerPool.executeNotify(() -> {
try {
sendNoticeMsg(getOneReceiverById(receiverId),
getOneTemplateById(rule.getTemplateId()), alert);
} catch (AlertNoticeException e) {
log.warn("DispatchTask sendNoticeMsg error, message: {}", e.getMessage());
}
});
})));
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: Add dedicated thread pool for RestTemplate in alert notification system
What's changed?
RestTemplate thread pool
Added a
restTemplateThreadPoolbean inhertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/config/RestTemplateConfig.javawith:
RestTemplate-CallerRunsPolicyAsync alert notification
In
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/AbstractAlertNotifyHandlerImpl.javaintroduced a
sendAsync()method and injected the new thread‐pool executor to offload HTTP calls.Parallel dispatch optimization
Updated
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/AlertNoticeDispatch.javato:
CompletableFuturefor sending notifications in parallelTest updates
In
hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/notice/AlertNoticeDispatchTest.javaupdated the constructor calls to pass a mock executor and added test cases covering the async paths.
Checklist
Add or update API