[feat] #315 - 회원가입 시 Slack으로 알림을 보내는 비동기 이벤트 리스너 구현 #316
Conversation
… 사용하기 위한 의존성 추가
…로 요청을 보내는 이벤트리스너 생성
…figurationProperties 추가
WalkthroughThe changes introduce new dependencies in the build configuration and update package structures to reorganize DTOs and imports. An annotation for scanning configuration properties is added to the main application, and service methods are enhanced with a count operation and event publishing on member registration. New classes configure asynchronous execution and thread pools while a global asynchronous exception handler is implemented. Slack integration is added with a Feign client, a service for sending messages, and an event listener to process member registration events. Additionally, environment-specific configuration files are updated with Slack and thread pool settings. Changes
Sequence Diagram(s)sequenceDiagram
participant MRS as MemberRegistrationService
participant EP as EventPublisher
participant MREL as MemberRegisteredEventListener
participant SS as SlackService
participant SC as SlackClient
MRS->>EP: Publish MemberRegisteredEvent (with nickname)
EP-->>MREL: Deliver event (after commit)
MREL->>SS: Call sendSlackNotification(event)
SS->>SC: sendMessage(payload)
SC-->>SS: Acknowledge receipt
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/beat/global/external/notification/slack/application/SlackClient.java (2)
1-15: Consider creating a dedicated DTO for Slack messagesThe interface is well-defined using OpenFeign for the Slack webhook integration. However, using a generic
Map<String, String>as the payload format might make the code less maintainable and type-safe.Consider creating a dedicated DTO class that represents the Slack message format:
- void sendMessage(@RequestBody Map<String, String> payload); + void sendMessage(@RequestBody SlackMessageRequest payload);And add a new class:
package com.beat.global.external.notification.slack.application.dto.request; public record SlackMessageRequest( String text // Add other Slack message properties as needed ) { public static SlackMessageRequest of(String text) { return new SlackMessageRequest(text); } }This would make the code more readable and maintainable, with the added benefit of documentation through code.
10-10: Consider adding connection timeout configurationWhen working with external services like Slack, it's a good practice to configure timeout settings to prevent long waits if the service is unresponsive.
- @FeignClient(name = "slackClient", url = "${slack.webhook.url}") + @FeignClient( + name = "slackClient", + url = "${slack.webhook.url}", + configuration = SlackClientConfiguration.class + )And create a configuration class:
package com.beat.global.external.notification.slack.application.config; import feign.Request; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import java.util.concurrent.TimeUnit; @Configuration public class SlackClientConfiguration { @Bean public Request.Options options() { return new Request.Options( 5, TimeUnit.SECONDS, // connection timeout 10, TimeUnit.SECONDS, // read timeout true // follow redirects ); } }src/main/resources/application-prod.yml (1)
121-123: Consider thread pool sizing for production workloads.While a core size of 2 may be sufficient initially, you should monitor thread pool utilization in production and adjust as needed based on actual workload.
Consider implementing metrics collection for your thread pool to help with capacity planning:
thread-pool: core-size: 2 thread-name-prefix: executor- + max-size: 10 + queue-capacity: 100src/main/java/com/beat/global/external/notification/slack/application/SlackService.java (1)
15-17: Consider adding error handling.The current implementation lacks error handling for cases where the Slack API might be unavailable or return errors.
Consider adding error handling to prevent issues with the Slack notification from affecting other operations:
public void sendMessage(Map<String, String> payload) { - slackClient.sendMessage(payload); + try { + slackClient.sendMessage(payload); + } catch (Exception e) { + // Log the error but don't propagate it + log.error("Failed to send Slack notification: {}", e.getMessage(), e); + } }Don't forget to add the Lombok @slf4j annotation to the class for logging.
src/main/java/com/beat/global/common/config/AsyncThreadConfig.java (1)
28-29: Consider setting additional executor propertiesThe current configuration only sets core pool size and thread name prefix. Consider adding:
- Maximum pool size (if different from core size)
- Queue capacity
- Keep-alive time for idle threads above core count
This would make the thread pool behavior more predictable under varying loads.
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); executor.setCorePoolSize(threadPoolProperties.getCoreSize()); + executor.setMaxPoolSize(threadPoolProperties.getMaxSize()); + executor.setQueueCapacity(threadPoolProperties.getQueueCapacity()); + executor.setKeepAliveSeconds(threadPoolProperties.getKeepAliveSeconds()); executor.setThreadNamePrefix(threadPoolProperties.getThreadNamePrefix());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
build.gradle(1 hunks)src/main/java/com/beat/BeatApplication.java(2 hunks)src/main/java/com/beat/domain/member/api/MemberApi.java(1 hunks)src/main/java/com/beat/domain/member/api/MemberController.java(1 hunks)src/main/java/com/beat/domain/member/application/AuthenticationService.java(1 hunks)src/main/java/com/beat/domain/member/application/MemberRegistrationService.java(4 hunks)src/main/java/com/beat/domain/member/application/MemberService.java(1 hunks)src/main/java/com/beat/domain/member/application/SocialLoginService.java(2 hunks)src/main/java/com/beat/domain/member/application/dto/event/MemberRegisteredEvent.java(1 hunks)src/main/java/com/beat/domain/member/application/dto/response/AccessTokenGenerateResponse.java(1 hunks)src/main/java/com/beat/domain/member/application/dto/response/LoginSuccessResponse.java(1 hunks)src/main/java/com/beat/domain/member/application/dto/response/MemberLoginResponse.java(1 hunks)src/main/java/com/beat/domain/member/port/in/MemberUseCase.java(1 hunks)src/main/java/com/beat/global/common/config/AsyncThreadConfig.java(1 hunks)src/main/java/com/beat/global/common/config/ThreadPoolProperties.java(1 hunks)src/main/java/com/beat/global/common/handler/GlobalAsyncExceptionHandler.java(1 hunks)src/main/java/com/beat/global/external/notification/slack/application/SlackClient.java(1 hunks)src/main/java/com/beat/global/external/notification/slack/application/SlackService.java(1 hunks)src/main/java/com/beat/global/external/notification/slack/event/MemberRegisteredEventListener.java(1 hunks)src/main/resources/application-dev.yml(1 hunks)src/main/resources/application-prod.yml(1 hunks)
🔇 Additional comments (25)
src/main/java/com/beat/domain/member/port/in/MemberUseCase.java (1)
14-15: LGTM! Good addition of countMembers method.This new method aligns well with the PR objective of providing Slack notifications, as it could be used to include the total member count in notifications upon new registrations.
src/main/java/com/beat/domain/member/application/dto/event/MemberRegisteredEvent.java (1)
1-6: Good implementation of event using Java record.The MemberRegisteredEvent record is a clean, immutable data structure that perfectly fits the event-driven design for Slack notifications. This implementation aligns well with the PR objective of sending Slack notifications upon user registration.
src/main/java/com/beat/domain/member/application/dto/response/LoginSuccessResponse.java (1)
1-1: Appropriate package restructuring.Moving this class to the
application.dto.responsepackage improves the organization of DTOs in the codebase, making the architecture more maintainable and clearer.src/main/java/com/beat/domain/member/application/dto/response/AccessTokenGenerateResponse.java (1)
1-1: Good package structure reorganization.Relocating this DTO to the
application.dto.responsepackage is consistent with the architectural improvements being made throughout the codebase.src/main/java/com/beat/domain/member/application/dto/response/MemberLoginResponse.java (1)
1-1: LGTM: Well-structured package nameThe updated package name follows a clean architecture approach, organizing DTOs by their layer (application) and purpose (response), which improves code organization and maintainability.
src/main/java/com/beat/BeatApplication.java (1)
6-6: Good addition of ConfigurationPropertiesScanThis annotation enables automatic detection of
@ConfigurationPropertiesclasses throughout the application, which will help with your Slack webhook configuration and thread pool settings. This is the correct approach for externalizing configuration in Spring applications.Also applies to: 18-18
build.gradle (1)
109-114: LGTM: Appropriate dependencies for Slack integrationThe Slack API client and Spring Boot Configuration Processor are appropriate dependencies for implementing the Slack notification feature.
src/main/java/com/beat/domain/member/application/SocialLoginService.java (1)
5-5: Import path update looks good.The updated import path follows a more structured organization pattern, moving the DTO to a dedicated response package within the application layer.
src/main/java/com/beat/domain/member/api/MemberApi.java (1)
8-9: Import path restructuring approved.These import changes maintain consistency with the overall package reorganization, properly relocating response DTOs to the application-specific package.
src/main/resources/application-dev.yml (2)
118-120: Slack webhook configuration looks good.The Slack webhook URL is properly configured using an environment variable which is a good security practice.
122-124: Thread pool configuration is appropriate for development.The thread pool configuration with a core size of 2 is suitable for a development environment. The thread name prefix will help with identifying these threads in logs and thread dumps.
Consider monitoring thread pool metrics in production to ensure the core size is appropriate for your workload. You might need to adjust it based on performance testing results.
src/main/resources/application-prod.yml (1)
117-119: Slack webhook configuration is properly set.Using environment variables for the Slack webhook URL in production is a good security practice.
src/main/java/com/beat/global/external/notification/slack/application/SlackService.java (1)
9-11: Service setup looks good.The service is properly annotated and follows the Spring dependency injection pattern.
src/main/java/com/beat/global/common/config/ThreadPoolProperties.java (1)
1-14: Well-structured configuration properties classThis class is correctly implemented to bind external configuration properties with the "thread-pool" prefix. The use of final fields ensures immutability, and Lombok annotations reduce boilerplate code.
The properties will be used to configure thread pools for asynchronous operations, which aligns well with the PR objective of implementing asynchronous Slack notifications.
src/main/java/com/beat/domain/member/api/MemberController.java (1)
15-17: Import path reorganization looks goodThe imports have been updated to reflect the new package structure for response DTOs, which improves code organization.
src/main/java/com/beat/domain/member/application/MemberRegistrationService.java (4)
3-3: New event import for async processingImporting the MemberRegisteredEvent class for event-driven processing of member registration.
14-14: ApplicationEventPublisher import for event publishingThis import enables the service to publish events throughout the application.
23-23: Event publisher dependency injectionThe ApplicationEventPublisher is properly injected as a final field through the constructor using Lombok's @requiredargsconstructor.
49-50: Event publication after member registrationThe implementation correctly publishes the MemberRegisteredEvent after the member is successfully registered. This aligns with the PR objective of implementing asynchronous Slack notifications without affecting the registration response time.
Note that the event is published within the transaction. Since you're using TransactionPhase.AFTER_COMMIT in your listener (as mentioned in the PR objectives), this ensures the notification is sent only after successful transaction completion.
src/main/java/com/beat/domain/member/application/MemberService.java (2)
46-46: Added @OverRide annotationExplicitly marking the method as implementing an interface method improves code clarity.
55-59: New method to count membersThis method is properly implemented with a read-only transaction annotation, which is appropriate for a query method. It will be used in the Slack notification to report the total member count after a new registration.
src/main/java/com/beat/domain/member/application/AuthenticationService.java (1)
3-4: Good DTO package restructuringThe import path changes reflect a logical reorganization of DTOs into a more structured package path. This aligns with clean architecture principles by better separating response DTOs in a dedicated package.
src/main/java/com/beat/global/external/notification/slack/event/MemberRegisteredEventListener.java (1)
29-31: Excellent async implementation with transaction awarenessThe combination of
@Async("taskExecutor")with@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)ensures that:
- The notification doesn't block the main registration flow
- The notification only executes after the transaction is successfully committed
- The notification won't execute if the transaction fails
This addresses the issue mentioned in the PR objectives where asynchronous tasks were executing before transaction commit.
src/main/java/com/beat/global/common/handler/GlobalAsyncExceptionHandler.java (1)
14-26: Well-implemented async exception handler with comprehensive loggingThe exception handler properly logs the method name, parameters, and exception details, making it easier to troubleshoot async execution failures. The null check for parameters is a good defensive programming practice.
src/main/java/com/beat/global/common/config/AsyncThreadConfig.java (1)
24-33: Well-configured thread pool with security context preservationThe async executor configuration is well implemented with:
- Configurable core pool size from properties
- Descriptive thread naming for easier debugging
- A
CallerRunsPolicyfor handling thread pool saturation- Security context preservation using
DelegatingSecurityContextExecutorThe
CallerRunsPolicyis particularly good as it provides backpressure by making the calling thread execute the task when the queue is full, preventing resource exhaustion.
| try { | ||
| slackService.sendMessage(payload); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(SLACK_TRANSFER_ERROR); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for non-critical notification failures
The current implementation throws a RuntimeException when Slack notification fails, which might not be appropriate for a non-critical background task. Consider logging the error instead of throwing an exception, since a failed notification shouldn't impact the core application flow.
try {
slackService.sendMessage(payload);
} catch (Exception e) {
- throw new RuntimeException(SLACK_TRANSFER_ERROR);
+ log.error(SLACK_TRANSFER_ERROR, e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| slackService.sendMessage(payload); | |
| } catch (Exception e) { | |
| throw new RuntimeException(SLACK_TRANSFER_ERROR); | |
| } | |
| try { | |
| slackService.sendMessage(payload); | |
| } catch (Exception e) { | |
| log.error(SLACK_TRANSFER_ERROR, e); | |
| } |
There was a problem hiding this comment.
Wouldn't just log.error() make it go to GlobalAsyncExceptionHandler and miss exception handling?
There was a problem hiding this comment.
@hoonyworld Actually, it would be the opposite - using log.error() would prevent the exception from reaching the GlobalAsyncExceptionHandler.
Here's how it works:
- The
GlobalAsyncExceptionHandleronly handles uncaught exceptions from@Asyncmethods - In your current code, you're catching the exception but then throwing a new
RuntimeException, which becomes an uncaught exception that will be handled byGlobalAsyncExceptionHandler - If you replace it with
log.error(), you're fully handling the exception within the method, so nothing propagates toGlobalAsyncExceptionHandler
If you want both logging and to have the exception handled by GlobalAsyncExceptionHandler, you could do:
try {
slackService.sendMessage(payload);
} catch (Exception e) {
log.error(SLACK_TRANSFER_ERROR, e);
throw new RuntimeException(SLACK_TRANSFER_ERROR, e);
}But for non-critical notifications, simply logging the error is usually sufficient since you don't want notification failures to impact core functionality.
There was a problem hiding this comment.
public class GlobalAsyncExceptionHandler implements AsyncUncaughtExceptionHandler {
@Override
public void handleUncaughtException(@NonNull Throwable ex, @NonNull Method method, @Nullable Object... params) {
if (params == null) {
log.error("비동기 작업 중 예외 발생! Method: [{}], Params: [null], Exception: [{}]",
method.getName(), ex.getMessage(), ex);
return;
}
String paramValues = Arrays.toString(params);
log.error("비동기 작업 중 예외 발생! Method: [{}], Params: [{}], Exception: [{}]",
method.getName(), paramValues, ex.getMessage(), ex);
}
}- 'GlobalAsyncExceptionHandler' is logging errors, so can I just throw RuntimeException without using log.error?
There was a problem hiding this comment.
@hoonyworld Yes, you're absolutely right. Since your GlobalAsyncExceptionHandler is already set up to log uncaught exceptions from @Async methods, you can simply throw the RuntimeException without adding a redundant log.error() call.
When you throw:
throw new RuntimeException(SLACK_TRANSFER_ERROR);The exception will propagate to the GlobalAsyncExceptionHandler, which will log the error with method name, parameters, and exception details.
This approach has the advantage of providing consistent error logging across all your async operations. Just be aware that when throwing the exception, you're marking the entire async operation as failed, but since this is a non-critical notification feature running after transaction commit, that's a reasonable approach.
Docstrings generation was requested by @hoonyworld. * #316 (comment) The following files were modified: * `src/main/java/com/beat/BeatApplication.java` * `src/main/java/com/beat/domain/member/application/MemberRegistrationService.java` * `src/main/java/com/beat/domain/member/application/MemberService.java` * `src/main/java/com/beat/domain/member/application/SocialLoginService.java` * `src/main/java/com/beat/domain/member/port/in/MemberUseCase.java` * `src/main/java/com/beat/global/common/config/AsyncThreadConfig.java` * `src/main/java/com/beat/global/common/handler/GlobalAsyncExceptionHandler.java` * `src/main/java/com/beat/global/external/notification/slack/application/SlackClient.java` * `src/main/java/com/beat/global/external/notification/slack/application/SlackService.java` * `src/main/java/com/beat/global/external/notification/slack/event/MemberRegisteredEventListener.java`
|
Note Generated docstrings for this pull request at #317 |
hyerinhwang-sailin
left a comment
hyerinhwang-sailin
left a comment
There was a problem hiding this comment.
,회원가입 알림 비동기 처리를 위해 고민하신 부분이 인상깊네요! 문서까지 공유해주셔서 잘 이해됐습니다~
Related issue 🛠
Work Description ✏️
상세 작업
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)을 이용해 커밋 이후에 이벤트 리스너가 작동할 수 있도록 구현하였습니다.Trouble Shooting ⚽️
해결 시 참고 링크
트랜잭션 커밋 이전에 비동기 작업이 일어나는 문제
🚨 문제 상황
🔍 원인 분석
✅ 해결 방법
Related ScreenShot 📷
Uncompleted Tasks 😅
To Reviewers 📢
링크
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor