[BOM-1031] feat: 연속 읽기 랭킹 조회 api#711
Hidden character warning
Conversation
Choidongjun0830
left a comment
There was a problem hiding this comment.
MemberContinueReadingRankResponse와 ContinueReadingRankResponse가 동일하다는 것은 MemberContinueReadingRankResponse에 앞 사람과의 격차 같은게 앞으로도 들어가지 않을 것이라면 같이 관리해도 괜찮을거 같습니다.
수고하셨습니다~! 감사합니다!
| List<ContinueReadingRankResponse> data | ||
| ) { | ||
|
|
||
| public static ContinueReadingRankingResponse of(List<ContinueReadingRankResponse> data) { |
There was a problem hiding this comment.
p1
이거 컨벤션 상 파라미터 하나일 경우에는 from이라서 수정 부탁드려용
There was a problem hiding this comment.
코드 수정에 따라 파라미터 여러개로 바뀌어서 그대로 of로 두겠습니다!
| @Schema(required = true) | ||
| long rank, | ||
|
|
||
| @Schema(required = true) | ||
| int dayCount, |
There was a problem hiding this comment.
p1
여기 required=true가 아마 deprecated?되어서 requireMode 사용해주시면 감사드리겠습니당
There was a problem hiding this comment.
reading 도메인에 있는 다른 코드들까지 다 적용했습니다
| ContinueReadingRankFlat flat = continueReadingRepository.findMemberContinueReadingRanking( | ||
| member.getId(), | ||
| lastMonthYear, | ||
| lastMonthValue | ||
| ); | ||
|
|
||
| if (flat == null) { | ||
| throw new CIllegalArgumentException(ErrorDetail.ENTITY_NOT_FOUND) | ||
| .addContext(ErrorContextKeys.MEMBER_ID, member.getId()) | ||
| .addContext(ErrorContextKeys.ENTITY_TYPE, "ContinueReading"); | ||
| } |
There was a problem hiding this comment.
p1
질문과 제안이 있습니다.
flat이 null이 되는 경우가 있다면 ContinueReadingFlat이 아닌 Optional<ContinueReadingFlat으로 받는게 어떨까요?
그리고, null인 경우는 아예 해당 멤버에 대한 ContinueReading이 없거나 streak이 끊겨 0으로 초기화된 경우인데요.
ContinueReading이 아예 없으면 애초에 회원가입 시점에 생성되지 않은 것이라 예외가 맞는데
streak이 0인 경우에 가장 마지막 순위가 아닌 404를 반환하는 의도가 궁금합니다!
streak이 0이면 끊긴 사람과 회원가입을 막 한 사람은 아예 404로 본인의 랭킹 정보를 보지 못할거 같습니다.
제 생각에는 월간 읽기 순위와 동일하게 가장 아래 공동 순위로 두는게 좋아보입니다.
그리고, PR 코멘트에서 스냅샷 테이블을 사용하지 않은 이유가 변경 주기 때문이라고 말씀하셨는데요. 제 생각에는 월간 읽기 순위에서 스냅샷 테이블을 둔 이유는 변경 주기가 아니라 조회 비용이 크기 때문이라고 생각합니다.
스냅샷의 존재 이유는 "데이터가 안 바뀌어서 캐시한다"가 아니라 "조회 비용 > 갱신 비용일 때 미리 계산해 둔다"라고 생각합니다. 현재 쿼리는 조회 시마다 전체 continue_reading 테이블에 윈도우 함수(RANK() OVER)를 실행하기 때문에 회원 수가 늘어날수록 매 요청마다 모든 회원을 확인해야 하는 비용이 발생할거 같아요!.
연속 읽기 데이터는 하루 한 번 바뀌지만, 랭킹 조회는 훨씬 자주 발생할 수 있으니 갱신 비용 대비 조회 비용이 더 큰 상황이라 스냅샷이 더 적합하다고 생각합니다!
There was a problem hiding this comment.
PR 코멘트에서 스냅샷 테이블을 사용하지 않은 이유가 변경 주기 때문이라고 말씀하셨는데요. 제 생각에는 월간 읽기 순위에서 스냅샷 테이블을 둔 이유는 변경 주기가 아니라 조회 비용이 크기 때문이라고 생각합니다.
👍
There was a problem hiding this comment.
Optional
refactor: 연속 읽기 순위 Optional 사용
404
스트릭이 0이면 404가 아니라 최하위순위가 되는게 맞네요. 감사합니다!
fix: 나의 연속 읽기 순위에서 dayCount 0을 최하위 공동 순위로 반환
스냅샷 도입
전체에 윈도우 함수를 도는 비용이 커지는 구조라, 갱신 비용을 감수하고 미리 계산해 두는 게 낫다는 이해에 동의합니다. 월간 랭킹과 동일한 패턴으로 수정했습니다.
관련커밋
| flat.nickname(), | ||
| flat.rank(), | ||
| flat.dayCount(), | ||
| BadgesResponse.from(flat) |
There was a problem hiding this comment.
질문이 있습니다!
스트릭 랭킹 뱃지를 따로 만드는거 같은데, 월간 랭킹 뱃지와 어떻게 구분되는건가요?
There was a problem hiding this comment.
회의에서 얘기한대로 BadgesResponse에 스트릭 랭킹 뱃지가 추가될 것 같습니다
There was a problem hiding this comment.
다른 브랜치에서 진행하는걸까요?? 기존 월간 랭킹 뱃지와 스트릭 랭킹 뱃지가 구분되지 않고 그냥 RankingBadge로만 쓰고 있는거 같아서요!
There was a problem hiding this comment.
이부분은 스트릭 뱃지 PR로 올릴 예정입니다
rladmstn
left a comment
There was a problem hiding this comment.
수고하셨습니다!
간단한 코멘트만 하나 남겨두었습니다.
저도 모루와 같은 의견인데, 랭킹 스냅샷이 존재하던 이유도 업데이트 주기 문제보다 '실시간 조회' 문제 때문에 도입했던 것으로 기억합니다. 매번 조회할 때마다 RANK() 계산 하면 회원이 늘 때마다 전체 회원 대상으로 랭킹을 계산하는 비용이 더 커질 것이기 때문인데요, 그래서 '조회 성능'을 위해서라도 연속 읽기 랭킹도 스냅샷이 도입되면 좋지않을까 싶습니다.
| public record ContinueReadingRankFlat( | ||
| String nickname, |
There was a problem hiding this comment.
반영했습니다
Choidongjun0830
left a comment
There was a problem hiding this comment.
수고하셨습니다~! 리뷰 코멘트 남겨놨습니다~!
| ORDER BY cb.created_at DESC | ||
| LIMIT 1 | ||
| ) cb_latest ON true | ||
| ORDER BY rs.rank_order ASC, m.nickname ASC |
There was a problem hiding this comment.
p1
ASC는 안써도 될거 같습니다!
There was a problem hiding this comment.
| ContinueReading newContinueReading = ContinueReading.create(memberId); | ||
| continueReadingRepository.save(newContinueReading); | ||
|
|
||
| continueReadingRankingSnapshotRepository.save( |
| } | ||
| } | ||
|
|
||
| private LocalDate lastMonthForRankingBadge() { |
There was a problem hiding this comment.
p4
메서드명 getLastMonth() 어떠신가용
There was a problem hiding this comment.
반영했습니다
| return LocalDate.now(clock).minusMonths(LAST_MONTH_OFFSET); | ||
| } | ||
|
|
||
| private ZonedDateTime serverZonedNowForRankingSchedule(ZoneId zoneId) { |
There was a problem hiding this comment.
p4
이거도 getCurrentServerZoneDateTime()로 해도 될 거 같아용
There was a problem hiding this comment.
| @Entity | ||
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class ContinueReadingRankingSnapshotMeta { |
There was a problem hiding this comment.
p3
기존에 존재하던 MonthlyReadingRankingSnapshotMeta 테이블과 합치면 어떨까 싶어요! 그 테이블에 데이터 한 행만 있고, 동일한 목적을 위해서 생긴 것이라 기존 테이블에 type 같은 컬럼을 추가해서 한테이블에서 각각을 관리할 수 있을거 같아요.
There was a problem hiding this comment.
이 부분도 고려해보았는데, 월간과 연속 읽기 랭킹이 앞으로도 동일한 성격으로 유지될지 확신하기 어려워, 우선은 분리된 구조를 유지하는 구조로 구현하였습니다. 이후 월간/연속 읽기 랭킹의 구조와 메타 정보가 계속 유사하고, 추가되는 랭킹들 또한 동일한 형태로 확장된다면 그 시점에 type 기반 통합 구조를 적용해보는 건 어떤가요?
There was a problem hiding this comment.
meta에 갱신하는 10분을 맞추기 위한 데이터만 있다고 알고 있는데, 혹시 나중에 어떻게 확장될 수 있다고 생각하신건지 궁금해요! 저는 상상이 안가서 ㅎㅎ 갱신 주기가 다른건 어차피 type으로 나눠져 있으니까 괜찮을거 같고요!
There was a problem hiding this comment.
저도 지금 당장은 meta 자체가 어떤 식으로 바뀔지 상상이 안가긴 하네요.
통합구조를 적용하고, 만약 나중에 달라질 일이 있다면 그 때 다시 분리를 해봐도 좋을 것 같습니다.
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @Table(name = "continue_reading_ranking_snapshot") | ||
| public class ContinueReadingRankingSnapshot { |
There was a problem hiding this comment.
p3
기존 월간 읽기 랭킹 도메인을 보면 실시간 집계는 MonthlyReadingRealtime, 배치 데이터는 MonthlyReadingSnapshot으로 네이밍되어 있습니다. 반면, 추가된 연속 읽기 도메인은 실시간 집계가 ContinueReading, 배치 데이터가 ContinueReadingRankingSnapshot
으로 서로 네이밍 규칙이 달라 한눈에 파악하기 다소 헷갈릴 수 있을거 같습니다. 일관성을 위해 네이밍을 기존 규칙과 일치시켜 주시면 어떨까용
There was a problem hiding this comment.
이건 원래 사용중이던 부분이 많아서 수정 사항이 좀 많네요
| flat.nickname(), | ||
| flat.rank(), | ||
| flat.dayCount(), | ||
| BadgesResponse.from(flat) |
There was a problem hiding this comment.
다른 브랜치에서 진행하는걸까요?? 기존 월간 랭킹 뱃지와 스트릭 랭킹 뱃지가 구분되지 않고 그냥 RankingBadge로만 쓰고 있는거 같아서요!
| @Id | ||
| @Column(nullable = false) | ||
| private Long memberId; |
There was a problem hiding this comment.
p1
ContinueReadingRankingSnapshot은 PK를 따로 안쓰고 memberId로 두셨네요!
이유가 따로 있는걸까요?? 요기만 그러면 헷갈릴 수 있을 것 같아서요!
There was a problem hiding this comment.
별다른 이유 없어서 구조 통일하는게 좋겠네요
| @RequiredArgsConstructor | ||
| public class ContinueReadingRankingSnapshotMetaService { |
There was a problem hiding this comment.
p1
컨벤션 차원으로 @Transactional(readOnly=true) 붙여주심 좋을 것 같아요!
There was a problem hiding this comment.
There was a problem hiding this comment.
기존에 저희 DTO에 @Schema 쓰는건 required만 표시했던 것 같은데 어느순간부터 이것저것 정보가 많이 들어가게 됐군요..
이번 PR에서도 type, format, description 유무가 다 다르게 있는데 하나로 통일하는건 어떤가요??
There was a problem hiding this comment.
사실 이것저것 들어가있던건 schema에 대한 컨벤션 정하기 전에 작성된 코드라서 그런 것 같네요.
앞으로도 PR에서 이전 코드들도 컨벤션 적용해서 올리는게 좋겠네요
|
|
||
| public ContinueReadingRankingSnapshot( |
There was a problem hiding this comment.
넣는게 맞네요!
Choidongjun0830
left a comment
There was a problem hiding this comment.
질문 하나 남겼습니다! 수고하셨습니다!
📌 What
연속 읽기(continue_reading.day_count) 기준 랭킹 목록·나의 순위 API 추가 (/streak/rank, /streak/rank/me).
월간 독서왕과 달리 갱신 시각 메타 없음, 스트릭 쪽에는 nextRankDifference 없음.
❓ Why
이달의 독서왕과 비슷한 방식으로 연속 독서왕을 노출하기 위함.
🔧 How
👀 Review Point (Optional)
불필요한 필드가 있기도 하고 확장성 측면에서 DTO를 분리하는게 낫다고 생각을 했는데, 두 응답 구조가 크게 달라질 일이 있나 싶어서 관리할 쿼리만 두배로 늘어나는거 아닌가 하는 고민이 있습니다.