Conversation
Test Results115 tests 115 ✅ 23s ⏱️ Results for commit ac0f755. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
따로 현재는 IN 상태에 대한 검증 로직만 있지만, 나중에 OUT이나 UPDATE 테스트 코드도 작성하면 좋을 것 같아요!
| .filter(seat -> | ||
| Duration.between(seat.getQueryTime(), LocalDateTime.now()).getSeconds() <= LIMIT_QUERY_TIME) |
There was a problem hiding this comment.
사소하지만..! 매번 localdateTime.now를 불러오지 않고
public List<SeatDto> getGeneralSeats() {
Collection<SeatDto> seatsValue = seats.values();
LocalDateTime now = LocalDateTime.now(); // 분리
return seatsValue.stream()
.filter(seat -> seat.getSubject().isNonMajor())
.filter(seat ->
Duration.between(seat.getQueryTime(), now).getSeconds() <= LIMIT_QUERY_TIME)
.sorted(Comparator.comparingInt(SeatDto::getSeatCount))
.toList();
}
stream 돌기 전으로 따로 빼놓으면 연산이 조금은 줄 수 있지 않을까요?
There was a problem hiding this comment.
그리고 해당 filter로직은 왜 추가된것인지 궁금합니다!
There was a problem hiding this comment.
최신 여석 정보만 필터링 하려는 의도입니다!
즉, 여석이 저장된 시간으로부터 3초 이내의 여석 정보만 가져오게 되는 거죠.
1-2분 전에 저장된 여석의 정보를 보내는 건 실시간 여석 전송에 의미가 없어지는 거니까요!
그리고 저번 변경 감지 정책을 최초로 도입했을 때, LIMIT_QUERY_TIME에 대한 논의가 이루어졌었는데 그때 당시 3초로 합의했었습니다!
boyekim
left a comment
boyekim
left a comment
There was a problem hiding this comment.
꼼꼼히 되살려주셨네여 감사함다
저는 좀 고민인게.,,.. ChangedSubjectBuffer 에서 getAllAndFlush를 실행할때 sjptProperties.getRequestPerSecondCount()를 제한숫자로 두거든요?? 왜냐면 buffer에 add도 되고 있으니까 무한정 가져오면서 죠기 갇혀버릴까봐 제한을 뒀어요... 이 생각이 틀렸는지부터 고민이고, sjptProperties.getRequestPerSecondCount() 이렇게 strict하게 두면 점점 옛날 정보가 쌓이고 그걸 계속 flush하게 될까봐 걱정이에오ㅠㅠ 그럼 결국 주기가 안맞아서 LIMIT_QUERY_TIME에도 필터링되어서 정보 갱신이 안될까봐 걱정이네요... 이걸 sjptProperties.getRequestPerSecondCount() 두배수 정도로 늘려야할지...흠
|
언급해주신 내용에 대한 논의가 예전에도 한 번 있었어서 같이 공유드립니다. 이후 해당 변경이 반영된 상태로 서비스를 운영했을 때,
오히려 여석 밀림 현상이 없어지고 서비스가 정상화 되었어요.
다만, 당시 경험 상 너무 strict한 flush 제한 수가 밀림 현상에 일정 부분 영향을 줬을 가능성은 있다고 보고 있고, 지표 수집추가적으로, 서비스 운영 시 해당 여석 갱신이 잘 되는지 여부를 확인하려면 직접 학사정보시스템의 여석과 비교해보는 것 외에는 마땅한 방법이 없었습니다. 그래서 여석 갱신 상태를 관찰할 수 있는 로그나 지표를 남기는 것이 좋겠다고 생각했습니다. 운영하는 입장에서도 좋고, 문제가 발생했을 때 어느 구간에서 병목이 생겼는지 트래킹하기도 훨씬 쉬울 것 같아서요! 예를 들면,
이런 식으로 지표를 쌓아두면, 여석이 실제로 잘 갱신되고 있는지를 정량적으로 확인하기도 쉽고 다시 밀림 현상이 생기더라도 어느 레이어에서 문제가 발생했는지 빠르게 추적할 수 있을 것 같아요!! 이에 대해 어떻게 생각하시는지 다른 분들의 의견이 궁금합니다! |
50개 정도도 좋을 것 같습니다! 그때 금서주스랑 해윤주스가 실제로 지표 찍어봤을 때 최대 30개(두배수)정도로 보였다고 말해줬던 것 같아서요
지표 수집 저도 말하고싶었어요 |
| if (canOut(crawlerSubject, remainSeat)) { | ||
| liveBoardSubjects.remove(crawlerSubject); | ||
| return List.of(ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.OUT, remainSeat)); | ||
| return List.of(ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.OUT, remainSeat, LocalDateTime.now())); | ||
| } |
There was a problem hiding this comment.
out 된 경우에는 전광판의 과목의 개수가 19개가 되니 21번째 전광판 과목이 20번째로 올라와야할 것 같은데 그 로직이 빠져있는 것 같아요!
out이 될 경우 in이 함께 동반되어야 할 것 같습니다. 어차피 몇 초 뒤면 새로운 과목들이 크롤링 되고, 과목이 20개가 채워질 수 도 있지만 새로운 과목이 나타나기 전까지는 19개만 전광판에 보일 것 같은데, liveboard 사이즈를 20보다 조금 크게 22-25 정도로 해두고 20개만 보이게 하는 것은 어떤가요?
There was a problem hiding this comment.
또한 checkStatus 메서드에 대해
public List<ChangeSubjectsResponse> checkStatus(CrawlerSubject crawlerSubject, Integer remainSeat) {
if (isRemainSeatBecomeEmpty(remainSeat)) { // 크롤링 한 과목의 여석이 0인 경우
if (canOut(crawlerSubject, remainSeat)) {
liveBoardSubjects.remove(crawlerSubject);
return List.of(ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.OUT, remainSeat,
LocalDateTime.now()));
}
} else { // 크롤링 한 과목의 여석이 0이 아닌 경우
if (canOnlyIn(crawlerSubject, remainSeat)) {
liveBoardSubjects.put(crawlerSubject, remainSeat);
return List.of(ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.IN, remainSeat,
LocalDateTime.now()));
}
CrawlerSubject maxCrawlerSubject = findMaxRemainSeatSubject();
Integer maxSubjectRemainSeat = liveBoardSubjects.get(maxCrawlerSubject);
if (canInAndOut(crawlerSubject, maxCrawlerSubject, remainSeat)) {
liveBoardSubjects.put(crawlerSubject, remainSeat);
liveBoardSubjects.remove(maxCrawlerSubject);
return List.of(
ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.IN, remainSeat, LocalDateTime.now()),
ChangeSubjectsResponse.of(maxCrawlerSubject.getId(), ChangeStatus.OUT, maxSubjectRemainSeat,
LocalDateTime.now())
);
}
if (canUpdate(crawlerSubject, remainSeat)) {
liveBoardSubjects.put(crawlerSubject, remainSeat);
return List.of(ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.UPDATE, remainSeat,
LocalDateTime.now()));
}
}
return Collections.emptyList();
}
과 같이 초반에 isRemainSeatBecomeEmpty(크롤링 한 과목의 여석이 0인지) 를 기준으로 분기처리를 해 로직을 수정하는 것은 어떤가요? 여석이 0인 경우에도 불필요하게 많은 로직들을 거치게 되는 것 같아서요!
There was a problem hiding this comment.
public void saveChangeToBuffer(CrawlerSubject crawlerSubject, CrawlerSeat renewedCrawlerSeat) {
if (isGeneralSubject(crawlerSubject)) { // 교양 과목인 경우 -> 대시보드
List<ChangeSubjectsResponse> response = liveBoards.checkStatus(
crawlerSubject,
SeatUtils.getRemainSeat(renewedCrawlerSeat)
);
if (!response.isEmpty()) { // 대시보드에 변화가 생긴 경우
changedSubjectBuffer.addAll(response);
}
} else { // 교양 과목이 아닌 경우 -> pin과목
changedSubjectBuffer.add(
ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.UPDATE, SeatUtils.getRemainSeat(
renewedCrawlerSeat), LocalDateTime.now()));
}
}changedSubjectBuffer.addAll(response); 에 걸리는 경우는 교양과목인 경우 중, 대시보드에 변화가 생긴 경우입니다.
else에 해당하는 changedSubjectBuffer.add()…. 가 실행되는 경우는 교양 과목이 아닌 경우 즉 pin 과목인 경우를 의도하고 작성된 부분인 것 같은데, 교양 과목인데 pin 과목인 경우에는 어느곳에도 해당하지 않아 여석이 업데이트 되지 않을 것 같습니다.
따라서
public void saveChangeToBuffer(CrawlerSubject crawlerSubject, CrawlerSeat renewedCrawlerSeat) {
if (isGeneralSubject(crawlerSubject)) { // 교양 과목인 경우 -> 대시보드
List<ChangeSubjectsResponse> response = liveBoards.checkStatus(
crawlerSubject,
SeatUtils.getRemainSeat(renewedCrawlerSeat)
);
if (!response.isEmpty()) { // 대시보드에 변화가 생긴 경우
changedSubjectBuffer.addAll(response);
return;
}
}
// 대시보드에 없는 교양 과목 + 교양 과목이 아닌 경우 -> pin과목
changedSubjectBuffer.add(
ChangeSubjectsResponse.of(crawlerSubject.getId(), ChangeStatus.UPDATE, SeatUtils.getRemainSeat(
renewedCrawlerSeat), LocalDateTime.now()));
}
}다음과 같이 로직을 변경해야하지 않나 하는 생각이 드는데 어떻게 생각하시나요?
| if (existingSeatOpt.isPresent()) { | ||
| CrawlerSeat existingCrawlerSeat = existingSeatOpt.get(); | ||
| existingCrawlerSeat.merge(crawlerSeat); |
There was a problem hiding this comment.
기존에는 과목을 크롤링할 때 마다 계속 save만 했던 로직을, 과목 여석 정보를 Update하는 방식으로 변경했는데요,
이는 매번 db에서 특정 과목의 여석 정보를 찾아, 현재 크롤링한 여석과 비교하여 변경 여부를 판단하기때문인 것 같습니다!
하지만 이는 매초 15번 일어나는 로직이기에
- db에서 특정 과목의 여석 정보를 찾아
- 과목 여석 정보를 Update하는 방식
요 경우(2) ==
Optional<CrawlerSeat> existingSeatOpt = crawlerSeatRepository.findByCrawlerSubjectAndCreatedDate(
crawlerSeat.getCrawlerSubject(),
today
);
에도 db에 부하를 너무 많이 주는 로직인 것 같습니다. 따라서 변경이 필요해보여요!
위에 링크 남겨놓은 부분이 db를 거치지 않는 방식으로 수정된다면, 해당 부분은 원래대로 save만 하는 방법으로 수정할 수 있을 것 같은데 어떻게 생각하시나요?
| Optional<CrawlerSeat> previousSeat = crawlerSeatRepository.findByCrawlerSubject(crawlerSubject); | ||
| LocalDate today = LocalDate.now(); | ||
|
|
||
| Optional<CrawlerSeat> previousSeat = crawlerSeatRepository.findByCrawlerSubjectAndCreatedDate(crawlerSubject, today); |
There was a problem hiding this comment.
db에서 매번 해당 과목의 여석 정보를 가져오는 것 보다, 모든 과목의 여석 정보를 담고있는 캐시에서 가져와서 비교하는 식으로 방법을 바꾸는건 어떨까요?
매번 db를 들락날락하면 병목이 생길 수 있을 것 같은데, 관리포인트가 너무 늘어나지 않는 선에서 방법을 고민해보면 좋을 것 같아요.
3 participants
작업 내용
여석 전송 로직을 변경했습니다.
기존에 보예님께서 작업하셨던 내용을 롤백한 것이므로, 작성하셨던 PR을 기반으로 이야기하겠습니다.
** <crawler - #7 PR>의 내용 기반으로 수정한 내용입니다. **
백엔드와 서버가 단일화 되어 더이상 SSE로 여석을 전송해주던 로직은 필요 없게 되었습니다.
따라서 SSE를 사용하는 것이 아닌 자료구조에 저장하는 방식으로 변경했습니다.
백엔드는 여석 크롤링 후 저장해둔 자료구조를 통해 여석 정보를 가져오게 될 것입니다. 여석을 저장할 자료구조의 이름은
ChangedSubjectBuffer입니다.변경된 여석 정보들이 담겨 있는
ChangedSubjectBuffer의getAllAndFlush()를 통해 버퍼에 저장된 것을 싹 가져가서 클라이언트에게 전송해줍니다. 그리고 전송한 과목들은 버퍼에서 내보냅니다.또한 data transfer 비용 문제로 변경 지점만 클라이언트에게 여석 전송을 하도록 합의했었습니다.
따라서 크롤러에서 변경지점이 있을 때에만 버퍼에 저장하고, DB 저장도 변경이 있을 때에만 update하도록 수정했습니다.
추가된 각 클래스의 역할은 다음과 같습니다.
ChangeDetectorisRemainSeatChanged메서드를 통해 여석의 변경을 감지합니다.saveChangeToBuffer를 통해 변경 지점을 버퍼에 저장해줍니다.LiveBoard에게 상태 판별을 한번 맡기고, 교양이 아닌 핀 과목이라면 무조건 UPDATE로 버퍼에 저장해줍니다.ChangeSubjectsResponseChangedSubjectBufferChangeSubjectsResponse들을 가지고 있습니다.LiveBoard고민 지점과 리뷰 포인트
1. 여석 정보 DB 저장 방안에 대한 논의가 필요합니다.
기존: 조회한 모든 여석 정보를 DB에 저장했습니다.
해당 방식 채택 근거
변경: 해당 과목에 대한 여석이 존재하면 update, 존재하지 않으면 insert 합니다.
해당 방식으로 변경한 이유
2. 여석 조회 시점의 데이터 정합성에 대해 논의가 필요합니다.