[DDING-50] SSE ACK API 구현 #208
Merged
Conversation
This file contains 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
Walkthrough이 풀 리퀘스트는 비디오 온디맨드(VOD) 처리 알림 시스템에 대한 주요 변경 사항을 포함하고 있습니다. 주요 변경 사항은 VOD 처리 알림을 위한 새로운 API, 컨트롤러, 서비스, DTO를 도입하여 알림 처리 메커니즘을 개선하는 것입니다. 서버 전송 이벤트(SSE)를 통해 VOD 처리 상태를 더 상세하게 전달하고, 알림 상태를 업데이트하는 기능을 추가했습니다. Changes
Suggested Labels
Suggested Reviewers
Possibly Related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingNotificationService.java (1)
9-10: 예외 처리에 대한 문서화가 필요합니다.
getById메서드가 존재하지 않는 ID로 조회될 경우의 예외 처리 방식을 문서화하면 좋을 것 같습니다. 예를 들어:
- 어떤 예외가 발생하는지
- 호출하는 쪽에서 어떻게 처리해야 하는지
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/VodProcessingNotificationController.java (1)
9-11: API 버전 관리를 고려해보세요.API 버전 관리를 위해
@RequestMapping어노테이션을 추가하는 것이 좋겠습니다:@RestController @RequiredArgsConstructor + @RequestMapping("/api/v1") public class VodProcessingNotificationController implements VodProcessingNotificationApi {src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/api/VodProcessingNotificationApi.java (1)
16-20: Swagger 문서를 더 상세하게 보완해주세요.현재 Operation과 ApiResponse의 설명이 다소 간단합니다. 다음 내용을 추가하면 좋을 것 같습니다:
- 요청 본문(request body)에 대한 설명
- 발생 가능한 오류 응답 코드 (예: 400, 404 등)
- 메서드의 동작 방식에 대한 자세한 설명
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingNotificationServiceImplTest.java (2)
23-41: 테스트 케이스 보완 필요다음과 같은 테스트 케이스를 추가하는 것이 좋습니다:
- 존재하지 않는 알림 ID에 대한 처리
- 이미 COMPLETED 상태인 알림에 대한 처리
- 동시에 여러 요청이 들어올 때의 동시성 처리
27-28: 메서드 이름 오타 수정 필요
creatPending()에 오타가 있습니다.createPending()으로 수정해야 합니다.src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java (1)
Line range hint
56-65: 트랜잭션 경계 검토 필요SSE 이벤트 전송이 트랜잭션 내에서 실행되고 있습니다. 다음 사항들을 고려해야 합니다:
- 트랜잭션 롤백 시 이벤트 처리 방안
- 이벤트 전송 실패 시 트랜잭션 처리 방안
- 장기 실행 트랜잭션 방지
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/api/VodProcessingNotificationApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/VodProcessingJobInternalController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/VodProcessingNotificationController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/dto/request/AckNotificationRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingNotification.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingNotificationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingNotificationServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingNotificationService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingNotificationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/UpdateAckedNotificationCommand.java(1 hunks)src/main/java/ddingdong/ddingdongBE/sse/api/SseConnectionApi.java(0 hunks)src/main/java/ddingdong/ddingdongBE/sse/service/dto/SseVodProcessingNotificationDto.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingNotificationServiceImplTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/ddingdong/ddingdongBE/sse/api/SseConnectionApi.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/VodProcessingJobInternalController.java
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/UpdateAckedNotificationCommand.java
- src/main/java/ddingdong/ddingdongBE/sse/service/dto/SseVodProcessingNotificationDto.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (2)
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingNotificationService.java (1)
5-8: 인터페이스 설계가 깔끔합니다!Facade 패턴과 Command 패턴을 적절히 활용하여 알림 업데이트 로직을 잘 캡슐화했습니다.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingNotificationService.java (1)
23-27: 구현이 적절해 보입니다!리소스를 찾을 수 없는 경우의 예외 처리와 에러 메시지가 명확하게 구현되어 있습니다.
Comment on lines
+10
to
+13
| public record AckNotificationRequest( | ||
| @Schema(description = "수신 완료 VodProcessingNotificaionId", example = "1") | ||
| Long vodNotificationId | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
입력값 유효성 검증이 필요합니다.
vodNotificationId에 대한 유효성 검증 어노테이션이 누락되었습니다. 다음과 같은 검증을 추가하는 것이 좋겠습니다:
public record AckNotificationRequest(
@Schema(description = "수신 완료 VodProcessingNotificaionId", example = "1")
+ @NotNull(message = "알림 ID는 필수입니다")
+ @Positive(message = "알림 ID는 양수여야 합니다")
Long vodNotificationId
) {Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+15
to
+18
| @Override | ||
| public void ackNotification(AckNotificationRequest request) { | ||
| facadeVodProcessingNotificationService.updateAckedNotification(request.toCommand()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
요청 검증과 예외 처리가 필요합니다.
- 요청 검증을 위한
@Valid어노테이션이 누락되었습니다. - 예외 상황(예: 존재하지 않는 알림 ID)에 대한 처리가 필요합니다.
@Override
- public void ackNotification(AckNotificationRequest request) {
+ public void ackNotification(@Valid AckNotificationRequest request) {
facadeVodProcessingNotificationService.updateAckedNotification(request.toCommand());
}예외 처리를 위한 글로벌 예외 핸들러나 컨트롤러 레벨의 예외 처리를 추가하는 것을 권장드립니다.
📝 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.
Suggested change
| @Override | |
| public void ackNotification(AckNotificationRequest request) { | |
| facadeVodProcessingNotificationService.updateAckedNotification(request.toCommand()); | |
| } | |
| @Override | |
| public void ackNotification(@Valid AckNotificationRequest request) { | |
| facadeVodProcessingNotificationService.updateAckedNotification(request.toCommand()); | |
| } |
Comment on lines
17
to
23
| @Override | ||
| public void updateAckedNotification(UpdateAckedNotificationCommand command) { | ||
| VodProcessingNotification vodNotification = | ||
| vodProcessingNotificationService.getById(command.vodNotificationId()); | ||
| vodNotification.updateVodNotification(VodNotificationStatus.COMPLETED); | ||
|
|
||
| } |
There was a problem hiding this comment.
트랜잭션 설정을 수정해주세요.
updateAckedNotification 메서드는 상태를 변경하는 작업을 수행하므로 다음과 같은 수정이 필요합니다:
- 메서드에
@Transactional어노테이션을 추가하여 읽기 전용 설정을 오버라이드 - 상태 변경 실패 시의 예외 처리 로직 추가
@Override
+ @Transactional
public void updateAckedNotification(UpdateAckedNotificationCommand command) {
VodProcessingNotification vodNotification =
vodProcessingNotificationService.getById(command.vodNotificationId());
vodNotification.updateVodNotification(VodNotificationStatus.COMPLETED);
}📝 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.
Suggested change
| @Override | |
| public void updateAckedNotification(UpdateAckedNotificationCommand command) { | |
| VodProcessingNotification vodNotification = | |
| vodProcessingNotificationService.getById(command.vodNotificationId()); | |
| vodNotification.updateVodNotification(VodNotificationStatus.COMPLETED); | |
| } | |
| @Override | |
| @Transactional | |
| public void updateAckedNotification(UpdateAckedNotificationCommand command) { | |
| VodProcessingNotification vodNotification = | |
| vodProcessingNotificationService.getById(command.vodNotificationId()); | |
| vodNotification.updateVodNotification(VodNotificationStatus.COMPLETED); | |
| } |
Comment on lines
+59
to
+61
| public void updateVodNotification(VodNotificationStatus vodNotificationStatus) { | ||
| this.vodNotificationStatus = vodNotificationStatus; | ||
| } |
There was a problem hiding this comment.
상태 전이 검증 로직이 필요합니다.
현재 구현은 어떤 상태로든 변경이 가능하여 잘못된 상태 전이가 발생할 수 있습니다. 다음과 같은 개선이 필요합니다:
- 허용된 상태 전이만 가능하도록 검증 로직 추가
- 잘못된 상태 전이 시도 시 예외 처리
예시 구현:
public void updateVodNotification(VodNotificationStatus vodNotificationStatus) {
+ validateStatusTransition(vodNotificationStatus);
this.vodNotificationStatus = vodNotificationStatus;
}
+ private void validateStatusTransition(VodNotificationStatus newStatus) {
+ if (!isValidTransition(this.vodNotificationStatus, newStatus)) {
+ throw new IllegalStateException(
+ String.format("Invalid status transition from %s to %s",
+ this.vodNotificationStatus, newStatus));
+ }
+ }
+ private boolean isValidTransition(VodNotificationStatus current, VodNotificationStatus next) {
+ return switch (current) {
+ case SENT -> next == VodNotificationStatus.COMPLETED;
+ default -> false;
+ };
+ }Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+68
to
74
| SseEvent<SseVodProcessingNotificationDto> sseEvent = SseEvent.of( | ||
| "vod-processing", | ||
| vodProcessingJob.getConvertJobStatus(), | ||
| new SseVodProcessingNotificationDto( | ||
| vodProcessingJob.getVodProcessingNotification().getId(), | ||
| vodProcessingJob.getConvertJobStatus()), | ||
| LocalDateTime.now() | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이벤트 실패 처리 로직 추가 필요
SSE 이벤트 전송 실패 시의 처리 로직이 누락되어 있습니다. 다음 사항들을 고려해야 합니다:
- 재시도 메커니즘
- 실패 로깅
- 클라이언트에게 오류 상태 전달
Comment on lines
+57
to
64
| SseEvent<SseVodProcessingNotificationDto> sseEvent = SseEvent.of( | ||
| "vod-processing", | ||
| vodProcessingJob.getConvertJobStatus(), | ||
| new SseVodProcessingNotificationDto( | ||
| vodProcessingJob.getVodProcessingNotification().getId(), | ||
| vodProcessingJob.getConvertJobStatus()), | ||
| LocalDateTime.now() | ||
| ); | ||
| sseConnectionService.sendVodProcessingNotification(vodProcessingJob, sseEvent); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
SSE 이벤트 생성 로직 중복 제거 필요
FacadeClubFeedServiceImpl와 동일한 SSE 이벤트 생성 로직이 중복되어 있습니다. 다음과 같은 개선이 필요합니다:
- 공통 유틸리티 클래스로 이벤트 생성 로직 추출
- 이벤트 타입 상수 정의
- 실패 처리 로직 통합
예시 리팩토링:
+ // SseEventFactory.java
+ public class SseEventFactory {
+ public static SseEvent<SseVodProcessingNotificationDto> createVodProcessingEvent(
+ VodProcessingNotification notification,
+ ConvertJobStatus status) {
+ return SseEvent.of(
+ "vod-processing",
+ new SseVodProcessingNotificationDto(notification.getId(), status),
+ LocalDateTime.now()
+ );
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
KoSeonJe
reviewed
Jan 12, 2025
...ong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingNotificationServiceImpl.java
Outdated
Show resolved
Hide resolved
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🚀 작업 내용
POST /server/vod-processing-notification/ack🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
릴리즈 노트
새로운 기능
개선 사항
기술적 변경