Repository navigation
[REFACTOR] 불필요한 트랜잭션 제거 - #39
Conversation
Summary by CodeRabbit릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings. Walkthrough더 이상 사용되지 않는 채팅 API 엔드포인트 및 응답 타입을 제거하고, 불필요한 Changes
추정 코드 리뷰 소요 시간🎯 2 (Simple) | ⏱️ ~10분
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chat/src/main/java/core/chat/service/ChatFacade.java (1)
93-112: 다중 엔티티 저장 시 트랜잭션 범위 검토 필요이 메서드는
chatRoomService.saveChatRoom(chatRoom)과chatHistoryService.saveChatHistory(userChat, llmChat)를 순차적으로 호출합니다.@Transactional제거 후 chatRoom 저장 성공 → chatHistory 저장 실패 시 고아 데이터가 발생할 수 있습니다.LLM 호출(line 97) 후에 DB 저장을 분리하는 방식으로 리팩토링하면, LLM 호출 중 커넥션 점유 문제와 데이터 정합성을 모두 해결할 수 있습니다.
// 예시: LLM 호출과 DB 저장 분리 public CreateChatRoomResponse startNewChat(String userId, CreateChatRoomRequest request) { Long roomId = Snowflake.getInstance().nextId(); String question = request.getQuestion(); // LLM 호출 (트랜잭션 외부) NewChatRoomInfo newChatRoomInfo = llmService.startNewChat(roomId, question); // DB 저장 (별도 트랜잭션 메서드로 분리) return saveNewChatData(roomId, userId, question, newChatRoomInfo); } @Transactional protected CreateChatRoomResponse saveNewChatData(Long roomId, String userId, String question, NewChatRoomInfo info) { // chatRoom, userChat, llmChat 저장 로직 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
chat/src/main/java/core/chat/controller/ChatController.java(0 hunks)chat/src/main/java/core/chat/controller/response/ChatResponseDeprecated.java(0 hunks)chat/src/main/java/core/chat/controller/response/CreateChatRoomResponseDeprecated.java(0 hunks)chat/src/main/java/core/chat/service/ChatFacade.java(1 hunks)mcp-client/src/main/java/core/mcpclient/service/LLMService.java(2 hunks)
💤 Files with no reviewable changes (3)
- chat/src/main/java/core/chat/controller/response/ChatResponseDeprecated.java
- chat/src/main/java/core/chat/controller/response/CreateChatRoomResponseDeprecated.java
- chat/src/main/java/core/chat/controller/ChatController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T07:10:22.381Z
Learnt from: kseysh
Repo: kseysh/factory-chatbot PR: 24
File: chat/src/main/java/core/chat/service/ChatFacade.java:96-100
Timestamp: 2025-11-20T07:10:22.381Z
Learning: In chat/src/main/java/core/chat/service/ChatFacade.java, the `checkUserCanDeleteRoom` method returns `orElse(true)` when a chat room doesn't exist. This is intentional to ensure idempotency of the DELETE operation - allowing deletion of non-existent rooms to succeed silently, treating DELETE as a declaration of desired end state rather than an action.
Applied to files:
chat/src/main/java/core/chat/service/ChatFacade.java
🔇 Additional comments (5)
chat/src/main/java/core/chat/service/ChatFacade.java (3)
54-54:boundedElastic스케줄러 사용 적절함블로킹 호출인
canUserAccessRoom을boundedElastic스케줄러에서 실행하여 리액티브 스트림의 논블로킹 특성을 유지한 좋은 패턴입니다.
197-205:deleteRoom에@Transactional유지 적절함삭제 작업은 LLM 호출이 없고,
chatRoom과chatHistory를 함께 삭제해야 하므로 트랜잭션 유지가 올바른 선택입니다.
29-47: No issue—ChatHistoryService.saveChatHistory()has@Transactionalannotation on the method itself (line 17), ensuring both user and LLM chat histories are saved atomically regardless of the@Transactionalremoval fromChatFacade.chat().mcp-client/src/main/java/core/mcpclient/service/LLMService.java (2)
42-49:@Transactional제거 적절함LLMService는 외부 LLM API를 호출하며 DB 작업을 수행하지 않습니다. 트랜잭션 제거로 LLM 응답 대기 중 커넥션 풀 점유 문제가 해결됩니다. PR 목표에 부합하는 변경입니다.
72-98: 스트리밍 응답에서 구분자 누락 시 동작 확인 필요
startNewChatStream에서는startNewChat과 달리 구분자(§)가 없을 때 예외를 던지지 않고 전체 chunk를 roomName으로 처리합니다 (line 87). 스트리밍 특성상 의도된 동작일 수 있으나, LLM이 구분자 없이 응답을 완료하면 answer가 빈 문자열이 될 수 있습니다.호출부(
ChatFacade.startNewChatStream)에서 빈 answer에 대한 처리가 있는지 확인이 필요합니다.
| if (responseContent == null) { | ||
| throw new RuntimeException("LLM 응답이 존재하지 않습니다."); | ||
| } | ||
| int separatorIndex = responseContent.indexOf(TITLE_SEPARATOR); | ||
| if (separatorIndex == -1) throw new RuntimeException("LLM의 답변에 제목과 본문을 구분하는 구분자가 존재하지 않습니다."); | ||
| if (separatorIndex == -1) { | ||
| throw new RuntimeException("LLM의 답변에 제목과 본문을 구분하는 구분자가 존재하지 않습니다."); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Null 체크 및 에러 처리 개선 승인
LLM 응답이 null이거나 구분자가 없는 경우에 대한 방어 로직 추가가 좋습니다. 다만, RuntimeException 대신 커스텀 예외나 보다 구체적인 예외 타입을 사용하면 호출부에서 에러 핸들링이 용이해집니다.
+// 예: LLMResponseException 커스텀 예외 사용
if (responseContent == null) {
- throw new RuntimeException("LLM 응답이 존재하지 않습니다.");
+ throw new LLMResponseException("LLM 응답이 존재하지 않습니다.");
}
int separatorIndex = responseContent.indexOf(TITLE_SEPARATOR);
if (separatorIndex == -1) {
- throw new RuntimeException("LLM의 답변에 제목과 본문을 구분하는 구분자가 존재하지 않습니다.");
+ throw new LLMResponseException("LLM의 답변에 제목과 본문을 구분하는 구분자가 존재하지 않습니다.");
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mcp-client/src/main/java/core/mcpclient/service/LLMService.java around lines
60 to 66, the code throws generic RuntimeException when responseContent is null
or missing the TITLE_SEPARATOR; replace these with a more specific, descriptive
exception type (e.g., create and throw a custom unchecked LLMResponseException
or use an appropriate existing exception like IllegalStateException) so callers
can handle errors explicitly; implement the new exception class in the same
package with constructors that accept a message and an optional cause, throw
that exception with clear messages (include relevant context such as the
response or separator value), and update any callers/tests that expect
RuntimeException to handle the new exception type.
Related issue 🛠
Work Description ✏️