feat: Add dedicated thread pool for RestTemplate in alert notificati… #3481
Conversation
…into RestTemplateConfig-Thread-pool
| executor.setCorePoolSize(10); | ||
| executor.setMaxPoolSize(50); | ||
| executor.setQueueCapacity(200); |
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);
| 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); | ||
| } |
There was a problem hiding this comment.
seem no others use this method.
There was a problem hiding this comment.
I have deleted this piece of code.
| 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()); | ||
| } | ||
| }); | ||
| }))); |
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.
|
hi suggest merge this in 1.7.3 after 1.7.2 released. |
| 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()); |
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());
}
});
})));
5 participants
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