-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#31] 경매 SSE 구독 및 가격 제안 기능 추가 #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 많으셨습니다!
피드백 확인 부탁드려요 ~
import com.example.readyauction.service.auction.AuctionService; | ||
|
||
@RestController | ||
@RequestMapping("/api/v1/auction") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url에는 보통 도메인을 복수형으로 표현해줘요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 넵 수정하겠습니다!
} | ||
|
||
// 경매 참여 API - SSE 구독 API | ||
@GetMapping(value = "/{productId}/subscribe", produces = MediaType.TEXT_EVENT_STREAM_VALUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 컨트롤러가 다루는 도메인은 auction이고 패스도 auction/
이라서 당연히 여기엔 auctionId가 오게 될거라고 생각할거에요 다른 도메인의 아이디가 필요할땐 앞에 도메인 이름을 붙여주는게 좋아요 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 넵! product 추가하도록 하겠습니다
private String userId; // 현재 최고가를 부른 유저 Id | ||
|
||
@Column(name = "user_Id", nullable = false, updatable = false) | ||
private int bestPrice; // 유저가 부른 최고가 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
최고가를 부른
유저가 부른 최고가겠죠 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 맞습니다! 주석이 약간 헷갈리게 되어있네용..
} | ||
|
||
@Override | ||
public boolean equals(Object o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 그런데 엔티티의 경우 id를 유니크하게 가지고 있으니 밑에껄 전부 비교해야 할 필요가 있을지 생각해보면 좋을 것 같아요 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 말씀하신것처럼 id는 user 엔티티마다 다 다르기때문에 전부 비교할필요 없이 id만 비교해도 될 것 같습니다.
수정하도록 하겠습니다!
private final Map<Long, Map<User, SseEmitter>> emittersMaps = new ConcurrentHashMap<>(); | ||
|
||
public void save(Long productId, User user, SseEmitter emitter) { | ||
Map<User, SseEmitter> emitters = emittersMaps.computeIfAbsent(productId, k -> new HashMap<>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동시성 문제를 고민하셔서 위에는 Concurrent를 사용하셨는데 여기는 그냥 HashMap을 써도 되나요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
외부 Map을 ConcurrentHashMap을 사용해서 내부는 HasnMap을 사용해도 되겠다라고 생각을 했었는데, 내부맵도 동시에 put이나 remove와 같은 작업이 이루어질 수 있겠다는 생각이 들어서 내부도 동일하게 ConcurrentHashMap으로 수정하겠습니다!
} else { // 있다는건 최고가를 제안한 사람이 있다는 것 | ||
Auction action = findAuction.get(); | ||
|
||
// 동시성 제어가 필요한 부분? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 부분에서 동시성 제어가 필요한지, 어떻게 제어할지도 혹시 생각 해 보셨을까요 ~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 사용자가 동시에 tenderPrice 메서드를 호출하고, 동일한 productId에 대해 입찰하는 경우에서 동시성 문제가 발생할 수 있다고 생각합니다.,
만약 첫 번째 사용자가 조회한 최고가가 1000원이고, 두 번째 사용자가 조회한 최고가도 1000원이라면, 둘 다 동시에 1000원 이상의 가격을 제안을 하겠다고 가정하면, 첫 번째 사용자가 1500원을 제안해서 성공적으로 최고가를 업데이트하고, 두 번째 사용자가 그 후에 최고가를 1200원으로 업데이트하는 상황이 발생할 수 있다고 생각이 듭니다.
방법은 현재 찾아보고 있습니다.
} | ||
} | ||
|
||
// 해당 경매에 참여한 User들에게 해당 경매 상품의 최고가가 갱신될때마다 SSE 알림. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
try { | ||
sseEmitter.send(SseEmitter.event() // SSE 이벤트를 생성하고 해당 Emitter로 전송합. | ||
.id(productId.toString()) // 이벤트의 고유 ID (문자열 형태로 변환) | ||
.name("BEST PRICE UPDATE") // 이벤트의 이름을 "sse"로 지정 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석이랑 동작이 좀 다른 것 같군요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 수정하겠습니다 ㅎ...
} | ||
|
||
private SseEmitter createEmitter(User user, Long productId, LocalDateTime closeDate) { | ||
// 경매가 끝나기 전까지 계속 최고가 알람을 줘야해서 경매 종료일 - 요청 시간 = SSE TimeOut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 굳이 이렇게 expire를 설정할 필요 있을까요 ? 그냥 경매 끝나는 액션 왔을때 해당 경매의 emitter 찾아서 종료시켜버리면 되는거 아닌가 싶어서요!
|
||
SseEmitter emitter = new SseEmitter(timeOut); | ||
emitterRepository.save(productId, user, emitter); | ||
// [나중에 추가 예정] 사용자가 "최고가 알람 끄기" 선택하면 delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
알람은 서버에서 꺼야하는건가 하는 생각이 드네요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 부분은 추후에 고민하고 있는 기능입니다.
단순하게 생각해서 예를들어 사용자가 설정에서 최고가 알람 받기 기능을 끄면 emitter를 삭제하도록 할려고했는데 이 부분은 조금 더 고민해보겠습니다!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 많으셨습니다! 요건 바로 머지 할게요 ~
[#31] 경매 SSE 구독 및 가격 제안 기능 추가