Skip to content
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

feat: 랭킹 API 구현 #391

Merged
merged 19 commits into from
Jun 15, 2024
Merged

feat: 랭킹 API 구현 #391

merged 19 commits into from
Jun 15, 2024

Conversation

char-yb
Copy link
Member

@char-yb char-yb commented May 6, 2024

🌱 관련 이슈

📌 작업 내용 및 특이사항

  • 랭킹 갱신을 위해 member 테이블보다는 ranking 테이블을 생성하여 활용도를 올리는 방향으로 선택
  • 랭킹 갱신 시간은 21시로, 초기 랭킹 데이터 생성과 갱신을 위해 insert duplicate Native-Query를 활용
  • 랭킹 테스트 코드

📝 참고사항

📚 기타

@char-yb char-yb linked an issue May 6, 2024 that may be closed by this pull request
@char-yb char-yb self-assigned this May 6, 2024
@char-yb char-yb added ✨ feature 새로운 기능 추가 및 수정 🥈 P2 급하지 않지만 꼭 필요한 이슈 labels May 6, 2024
@char-yb char-yb marked this pull request as ready for review May 7, 2024 08:10
@char-yb char-yb requested review from kdomo and uwoobeat as code owners May 7, 2024 08:10
Copy link
Member

@kdomo kdomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선 코멘트 먼저 드립니다😉

Comment on lines 22 to 23
void updateSymbolStackAndMemberId(
@Param("memberId") Long memberId, @Param("symbolStack") long symbolStack);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파라미터 순서와 메소드명 일치 시키면 더 좋을 것 같아욥

Comment on lines 29 to 35
public void updateSymbolStack(List<RankingDto> rankingDtos) {
for (RankingDto rankingDto : rankingDtos) {
Ranking ranking = Ranking.createRanking(rankingDto.symbolStack(), rankingDto.member());
rankingRepository.updateSymbolStackAndMemberId(
ranking.getMember().getId(), ranking.getSymbolStack());
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 메소드 설명 가능할까용?
랭킹 Entity를 생성하고, updateSymbolStackAndMemberId 메소드를 사용하는데,
insert를 하고있어서 약간 메소드명도 수정되어야 할 것 같고,
DUPLICATE KEY UPDATE를 할 때 NOW()를 실행한게 동일한 시간으로 판단되어서 중복으로 보는지도 궁금하네용

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 메서드는 SymbolStack을 약속한 시간에 갱신하기 위한 메서드이며 스케줄러에서만 사용되는 메서드입니당
rankingDtos는 findAllMissionSymbolStack 메서드로 인한 멤버 별 SymbolStack을 구한 List DTO로 해당 데이터를 갱신하고 있어요

좀 더 명확하게는 duplicate라고 메서드 명을 수정하는 게 낫겟네용👍

update를 할때 NOW()를 실행한 것을 보실때 쿼리가

@Query(
            value =
                    "INSERT INTO Ranking (member_id, symbol_stack, created_at) "
                            + "VALUES (:memberId, :symbolStack, NOW()) "
                            + "ON DUPLICATE KEY UPDATE member_id = :memberId, symbol_stack = :symbolStack, updated_at = NOW()",
            nativeQuery = true)

이처럼 insert 시 created_at에만 NOW(), update 시에는 updated_at에만 NOW() 실행하도록 하고 있어용
동일한 시간으로 판단되어서 중복으로 보는지에 대한 의미에 대해 다시 알 수 있을까요??

Comment on lines +18 to +20
"INSERT INTO Ranking (member_id, symbol_stack, created_at) "
+ "VALUES (:memberId, :symbolStack, NOW()) "
+ "ON DUPLICATE KEY UPDATE member_id = :memberId, symbol_stack = :symbolStack, updated_at = NOW()",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOW()를 실행하는 시점에 데이터가 있을때만 업데이트 되는거로 인지했어서 여쭤봅니당

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 데이터가 있을때만 업데이트되어 업데이트 시간만을 NOW()로 갱신해줍니당

Comment on lines 212 to 224
@Transactional(readOnly = true)
public List<RankingDto> findAllMissionSymbolStack() {
List<Mission> missions = missionRepository.findAllMissionWithRecords();
List<MissionRecord> completedMissionRecords = findCompletedMissionRecords(missions);

return completedMissionRecords.stream()
.collect(Collectors.groupingBy(MissionRecord::getMember))
.entrySet()
.stream()
.map(entry -> RankingDto.of(entry.getKey(), symbolStackCalculate(entry.getValue())))
.collect(Collectors.toList());
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미션서비스보다 랭킹 서비스에 들어가는게 더 적합해보이는데 어떠세염?

@char-yb
Copy link
Member Author

char-yb commented May 8, 2024

@kdomo 사실 이 부분을 개발하면서 약간 걸렸던게 21시에 스케줄 실행 시
duplicate 쿼리문이 symbolStack이 있는 멤버 수만큼 실행되서 혹시나 쿼리 수 감소나 성능을 최적화할 방법이 있을까요?.?

@kdomo
Copy link
Member

kdomo commented May 9, 2024

아 제가 로직을 잘못이해하고 있었군요,
윤범님이 짜신 로직이
첫째날, 21시에 랭킹테이블에 최초 1회 데이터 생성
둘째날, 21시에 기존에 있는 데이터에 심볼스택과, updatedAt을 업데이트
이거군요,

저는 날짜별로 랭킹에 대한 정보를 남겨놔야된다고 생각하고 있었어서,
업데이트를 왜 하는거지? 라는 의문이 있어서 질문 남겼던거였어용(머쓱)

기획상 필요하지 않을지 모르겠지만 윤범님이 구현하신 대로라면,
특정 과거일자에 랭킹에 들었던 유저를 찾고 싶다면 어떻게 해야할까요?

@kdomo
Copy link
Member

kdomo commented May 9, 2024

@kdomo 사실 이 부분을 개발하면서 약간 걸렸던게 21시에 스케줄 실행 시 duplicate 쿼리문이 symbolStack이 있는 멤버 수만큼 실행되서 혹시나 쿼리 수 감소나 성능을 최적화할 방법이 있을까요?.?

미션레코드에 존재하는 duration값으로 simbolStack을 계산하고 있으니, 지금 설계대로라면 딱히 방법이 떠오르진 않는군요,
우선 데이터가 그렇게 많지 않아서 문제가 발생할 것 같진 않아보이는데,
스케줄이 동작하는 시간(21시)에 서버 지표를 좀 확인해봐야할 것 같아용
만약 서버 인스턴스에 부하가 걸린다면 스케줄을 분리하는 쪽도 고려해 볼 수 있을 것 같네욤

  • 랭킹 데이터가 21시에 업데이트되고, 변하지 않는 값이니 24시간 동안 캐싱 되어도 좋을 것 같아용
    회의 때 논의해보시죠 😆

Copy link
Member

@uwoobeat uwoobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트도 같이 작성해주시면 좋을듯 합니다

import com.depromeet.domain.ranking.domain.Ranking;

public interface RankingRepositoryCustom {
void saveOrUpdate(Ranking ranking);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RepositoryCustom은 QueryDSL 기능을 사용하기 위한 목적을 가지는 것이 아닌가요?
왜 saveAndUpdate가 들어갔는지 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미사용 메서드입니당
이전 랭킹 데이터를 효율적으로 저장하는 것에 대해 고민하다 그대로 두었네용😅


public record RankingResponse(
@Schema(description = "사용자 ID", defaultValue = "1") Long memberId,
@Schema(description = "랭킹", defaultValue = "1") long rank,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

멤버 ID에서는 Long을 사용하고, 랭크 및 기타 필드에서는 long을 사용하신 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 memberId는 일관성 있게 Long으로 사용하였고,
long을 사용하는 이유는 굳이 기타 필드에 참조 타입 Long을 사용할 필요가 없다고 생각했어용
만약 rank나 symbolStack이 int의 범위만으로 충분하다면 int로 바꾸는 것도 고려할만하겠네요!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 그래도 박싱 / 언박싱 비용 생각해서 일관성 있게 전부 참조타입으로 맞춰주는게�좋지 않을까 싶네요
이거는 선택적으로 적용하시면 될듯 합니다

@char-yb
Copy link
Member Author

char-yb commented May 19, 2024

@kdomo

특정 과거일자에 랭킹에 들었던 유저를 찾고 싶다면 어떻게 해야할까요?

저도 기획상 필요성에 대해 느껴지지 않지만, 만약 필요하다면
특정 과거일자의 사용자 랭킹을 찾는다면 아마 save로만 진행하여 일자별 데이터를 저장하도록 해야할 듯해용

Copy link

Copy link
Member

@uwoobeat uwoobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@kdomo kdomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@char-yb char-yb merged commit 07ba94d into develop Jun 15, 2024
2 checks passed
@github-actions github-actions bot added the merged 머지된 PR label Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature 새로운 기능 추가 및 수정 merged 머지된 PR 🥈 P2 급하지 않지만 꼭 필요한 이슈
Projects
Status: 완료
Development

Successfully merging this pull request may close these issues.

✨ 랭킹 API 구현
3 participants