fix: 키워드 알림 스레드 점유 문제 개선#2236
Conversation
… fix/2191-keyword-notification-thread # Conflicts: # src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java # src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
dh2906
left a comment
dh2906
left a comment
There was a problem hiding this comment.
고생하셨습니다!
아래는 제가 스레드 풀 설정 관련 궁금한 내용을 질문으로 남깁니다!!
ThreadPoolTaskExecutor는 유저에게 알림을 보내기 위한 스레드 풀이 맞나요?- 1번이 맞다면
corePoolSize값은 결국 게시글 1개에 키워드가 매칭된 유저가n명이면 알림을n번 보내야 하니 "게시글 하나에 매칭된 유저가 몇 명인지" 를 기준으로 봐야하는 것 아닌가? 라는 생각이 듭니다.
Soundbar91
left a comment
Soundbar91
left a comment
There was a problem hiding this comment.
ThreadPoolTaskExecutor는 유저에게 알림을 보내기 위한 스레드 풀이 맞나요?
키워드가 매칭된 게시글에 대해서 발행된 이벤트를 처리하기 위한 스레드 풀입니다 ! 게시글이 4개가 들어왔을 때, 키워드가 매칭된 게시글이 2개가 존재한다면 2개의 알림 이벤트가 발생되고 이를 위한 스레드를 스레드 풀에서 생성해서 가져옵니다.
PR를 보니 설명이 많이 부족한 감이 있었네요.. 다음 PR부터는 보완하겠습니당
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughArticleKeywordEventListener is reconfigured to execute keyword event processing asynchronously via a dedicated ThreadPoolTaskExecutor bean. Keyword event handling is deferred until after the surrounding database transaction commits, separating Tomcat request thread handling from background FCM notification delivery. ChangesAsync Keyword Notifications
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
src/main/java/in/koreatech/koin/global/config/AsyncConfig.java (1)
34-34:CallerRunsPolicycan reintroduce the original Tomcat thread-occupancy problem under saturation.When the executor is fully saturated (
maxPoolSize=6threads busy +queueCapacity=5slots full = 11 concurrent tasks), the 12th submission is executed directly by the submitting thread. In Spring's async event infrastructure, that submitting thread is the post-commit synchronization thread — effectively the Tomcat request thread. Under a burst of keyword-match events, this silently falls back to synchronous FCM execution on the request thread, which is exactly the behaviour this PR aims to eliminate.Consider whether a larger
queueCapacity, anAbortPolicywith a logged drop metric, or a secondary bounded queue + async drain would better meet the resilience requirement without the silent fall-back.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/global/config/AsyncConfig.java` at line 34, The current use of CallerRunsPolicy in AsyncConfig (executor.setRejectedExecutionHandler(new CallerRunsPolicy())) can force work onto the Tomcat request thread; replace it with a bounded-failure strategy: change the rejected handler to ThreadPoolExecutor.AbortPolicy or a custom RejectedExecutionHandler that logs the rejection and increments a drop metric (e.g., implement new RejectedExecutionHandler that calls metrics.counter("async.rejected").increment() and processLogger.warn(...) before throwing RejectedExecutionException), or alternatively increase queueCapacity on the executor to a higher sensible value; update the executor configuration (in AsyncConfig where setRejectedExecutionHandler is called) to use one of these approaches to avoid silent synchronous fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/in/koreatech/koin/global/config/AsyncConfig.java`:
- Around line 26-37: The ThreadPoolTaskExecutor bean
keywordNotificationTaskExecutor currently doesn't wait for in-flight tasks on
shutdown; update the bean construction to call
executor.setWaitForTasksToCompleteOnShutdown(true) and set an appropriate
timeout via executor.setAwaitTerminationSeconds(<seconds>) so notification FCM
calls and keywordService.createNotifiedArticleStatus() complete during shutdown;
ensure these calls are applied on the ThreadPoolTaskExecutor instance before
executor.initialize() to enable graceful shutdown.
---
Nitpick comments:
In `@src/main/java/in/koreatech/koin/global/config/AsyncConfig.java`:
- Line 34: The current use of CallerRunsPolicy in AsyncConfig
(executor.setRejectedExecutionHandler(new CallerRunsPolicy())) can force work
onto the Tomcat request thread; replace it with a bounded-failure strategy:
change the rejected handler to ThreadPoolExecutor.AbortPolicy or a custom
RejectedExecutionHandler that logs the rejection and increments a drop metric
(e.g., implement new RejectedExecutionHandler that calls
metrics.counter("async.rejected").increment() and processLogger.warn(...) before
throwing RejectedExecutionException), or alternatively increase queueCapacity on
the executor to a higher sensible value; update the executor configuration (in
AsyncConfig where setRejectedExecutionHandler is called) to use one of these
approaches to avoid silent synchronous fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 143d4e16-096e-4590-b81f-30ae4f6123b3
📒 Files selected for processing (3)
src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javasrc/main/java/in/koreatech/koin/global/config/AsyncConfig.javasrc/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
…read' into fix/2191-keyword-notification-thread
아하! 이해했습니다 감사합니당 |
2 participants
🔍 개요
🚀 주요 변경 내용
키워드 알림 전송 로직 비동기 처리
ArticleKeywordEventListener에@Async("keywordNotificationTaskExecutor")적용POST /articles/keyword/notification요청을 처리하는 Tomcat 스레드가 FCM 전송 완료까지 점유되지 않도록 분리키워드 알림 전용 ThreadPoolTaskExecutor 추가
corePoolSize: 2maxPoolSize: 6queueCapacity: 5keepAliveSeconds: 60threadNamePrefix: keyword-notification-RejectedExecutionHandler: CallerRunsPolicy-> 초기값을 여유롭게 잡았으며, 프로덕션에 적용을 한 후 모니터링을 통해 값을 수정할 예정입니다.
💬 참고 사항
✅ Checklist (완료 조건)
Summary by CodeRabbit