Feature/283 봉사 시간 랭킹 기능 구현 #310
Conversation
- 기존 봉사 기록 생성 유스케이스와 구현체에 대한 네이밍 수
- sql 의존성 추가
- 본 클래스의 컨벤션에 맞춘 네이밍 수정에 따라 테스트 클래스의 명칭도 수정
- 레포지토리 인터페이스에 전체, 월, 주간 봉사 시간 랭킹 메서드 추가 - Jpa 레포지토리에 native쿼리 메서드 추가 - 레포지토리 구현체에 인터페이스 기능 구현 - 테스트 작성및 검증 완료
- 랭킹 계산 유스케이스 인터페이스 생성 - 구현체 생성및 랭킹 계산 메서드 구현 - 전체, 월, 주간 랭킹을 담는 Dto 클래스 구현 - 랭킹 객체 -> 랭킹 Dto mapper 구현 - 모든 랭킹을 담는 record 구현 - 테스트 작성및 검증 완료
- Redis 레포지토리 생성및 랭킹 저장 메서드 구현 - 랭킹 캐싱 유스케이스 생성 - 구현체 구현 - 테스트 작성및 검증 완료
- 랭킹 계산 유스케이스, 캐싱 유스케이스가 주입된 스케줄러 구현 - 테스트 작성및 검증 완료
- 유스케이스 인터페이스 생성 - 구현체 구현 - 테스트 작성및 검증 완료
- 기존 H2에서 Mysql로 변경
- 이전 커밋에서 누락된 테스트 클래스 추가
- 테스트 환경에서 mysql이용을 위한 test container 추가 - 관련 의존성 추가 - 관련 설정 클래스 생성 - 기존 support 클래스에 test container 클래스 상속 - 로그 제한을 위한 logback-test 추가
- 기존 h2, mysql 설정 제거
- 네이티브 쿼리 명시 - 맵퍼 클래스 수정 - 테스트및 검증 완료
- 봉사 랭킹 컨트롤러 구현 - 테스트및 검증 완료
- 봉사 시간 집계 실행, 종료 확인을 위한 로그 추
- 의미가 알맞은 예외 메세지로 수
- 레포지토리 인터페이스에 닉네임 리스트 반환 메서드 추가 - 레포지토리 구현체에 닉네임 리스트 반환 메서드 추가 - 테스트및 검증 완료
- 유스케이스 인터페이스 추가 - 구현체 생성 - 닉네임 리스트 반환 메서드 주입 - 테스트및 검증 완료
- Nickname앞에 volunteer를 추가하여 명시적인 이름으로 수정
- 봉사자 nickname을 담을 필드 추가
- volunteerId를 함께 반환 하도록 리팩토링
- volunteerId를 함께 반환 하도록 리팩토링
- 닉네임을 테스트 조건에 추가
- nickname을 가진 map에서 volunteerId로 값을 가져오는 로직추가 - toUUID 컨버터의 접근 제어자를 public으로 수정 - 테스트및 검증 완료
- volunteer nickname 추가에 따른 내부 로직 수정 - 봉사자 id 추출 로직 메서드로 분리 - 닉네임 추출 로직 메서드로 분리 - 테스트및 검증 완료
- redis, mysql test container 클래스 추가
- VolunteerRepositoryImpl 수정 - VolunteerRepositoryImplTest 수정
- 도커 권한 부여 step 추가 - 기존 ci에서 push 옵션 삭제
- testcontainer를 위한 설정추가
- testcontainer를 위한 설정추가
- testcontainer를 위한 설정추가
- 월요일이 주의 시작으로 계산되어 발생하는 테스트 에러 수정
- ci workflow 수정
- 스레드 풀의 크기를 16으로 감소
- cicd중 발생하는 테스트 에러 파악을 위해 추가했던 옵션 삭제
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
github-actions
bot
changed the title
- 스레드 에러 발생 테스트 주석처리
7zrv
changed the title
- 불필요한 코드, import문 제거 - 마지막 줄 개행이 없는 클래스 개행 추가
m-a-king
left a comment
m-a-king
left a comment
There was a problem hiding this comment.
수고하셨습니다!
마지막으로 여쭤보고 싶은 점은 "월간, 주간식으로 나누어진 랭킹 레코드(DTO)들은 변수 명으로 관리되고 하나의 클래스로 구성해도 되지 않을까?" 입니다~
| push: | ||
| branches: | ||
| [ main ] |
There was a problem hiding this comment.
push 는 왜 지우셨는지 여쭤보고 싶습니다~
There was a problem hiding this comment.
같은 로직을 두번 반복하는게 효율적이지 못하다는 생각이 들었고
이미 pr에서 CI를 하며 검증이 되었다고 생각해서 지웠습니다!
|
|
||
| private final RedisTemplate<String, Object> redisTemplate; | ||
|
|
||
| private static final Duration CACHE_TTL = Duration.ofMinutes(60); |
| redisTemplate.opsForValue().set(WEEKLY_RANKING_KEY, rankings.volunteerWeeklyRankingResponse(), CACHE_TTL); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Json 직렬화시 나오는 무시해도 되는 경고 문구를 제거하려고 어노테이션을 추가했습니다!
혹시나 팀원분들이 로그 확인하실때 찜찜하실까봐 설정했습니다
| if (totalRanking == null || monthlyRanking == null || weeklyRanking == null) { | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
하나라도 비어있으면 보내지 않으시는 이유가 있으신가요 ?
There was a problem hiding this comment.
프론트에서 탭으로 랭킹을 구분해서 보여주기때문에 월, 주간 같이 다른 탭을 클릭해야
볼 수 있는 랭킹의 경우 에러가 생겨도 발견이 늦을거 같다는 생각을 했습니다.
그래서 빠른 에러 파악을 위해 하나라도 문제가 생기면 빈 리스트를 반환하도록 했습니다
There was a problem hiding this comment.
여러 해결법이 있겠지만 좋다고 생각합니다~ 확인했습니다
| @Query(value = """ | ||
| SELECT volunteerId, totalHours, ranking | ||
| FROM ( | ||
| SELECT | ||
| vr.volunteer_id AS volunteerId, | ||
| SUM(vr.volunteer_hours) AS totalHours, | ||
| DENSE_RANK() OVER (ORDER BY SUM(vr.volunteer_hours) DESC) AS ranking | ||
| FROM volunteer_record vr | ||
| GROUP BY vr.volunteer_id | ||
| ) ranked | ||
| WHERE ranking <= 4 | ||
| """, nativeQuery = true) | ||
| List<Object[]> findTotalTopRankingWithTies(); |
There was a problem hiding this comment.
dense rank 와 같은 기능을 위해서 네이티브 쿼리를 활용하는 예시를 잘 확인했습니다!
혹시 일반적인 조회 기능정도의 역할을 데이터베이스에게 부여하고, 나머지는 자바로 처리하는 것은 어떤지 여쭤보고 싶습니다.
There was a problem hiding this comment.
일반적인 조회 기능정도의 역할을 데이터베이스에게 부여
이 부분에 대해서도 고민을 했었는데요 결국 랭킹은 모든 데이터를 추합해서 계산을 해야하기 때문에
앱에서 처리를 진행할 시 결국 findAll을 해와야 한다는 문제가 있었습니다
데이터 수가 많아질 때 반드시 문제가 될 부분이라 생각해서 다른 방식을 생각했습니다.
DB에서 뷰 테이블을 만들고 뷰 테이블을 조회하는 방식도 생각을 했으나
공부가 더 필요한 부분이라 지금의 쿼리 방식을 선택하게 되었습니다.
There was a problem hiding this comment.
네, 맞습니다. 저는 "네이티브 쿼리를 사용하지 않는 방식"에 대해서도 고민해보았습니다.
예를 들어, 봉사 시간을 기준으로 상위 10명을 조회한 후, Java에서 추가적으로 순위를 매기고, 공동 순위를 고려해 4등까지의 데이터를 추출하는 방식입니다.
(10명을 가져왔을 떄, 공동 순위를 고려한 상황에서 5등이 정해지지 않는다면 10+N명 정도를 더 조회하는 방법((20+N)명이 조회된 상황)으로 진행할 수 있다고 생각했습니다.)
이 접근 방식은 일반적으로 충분히 효율적일 수 있지만, 최악의 경우, 예를 들어 공동 1등이 10,000명인 상황에서는 비효율적일 수 있다는 문제가 있습니다.
모든 상황을 고려했을 때, 데이터베이스에서 적절히 필터링된 데이터를 가져오는 현재의 네이티브 쿼리 방식이 더 적합할 것 같습니다.
|
|
||
| List<List<Object[]>> rankings = List.of(volunteerTotalRanking, volunteerMonthlyRanking, volunteerWeeklyRanking); | ||
|
|
||
| Map<UUID, String> nicknameMap = createNicknameMap(rankings); |
There was a problem hiding this comment.
변수 명에 자료구조의 이름이 들어가는 것을 어떻게 생각하시나요?
저는 xxxMap보다는 aToB를 사용하려고 해서 여쭤봅니다~
There was a problem hiding this comment.
저번에도 말씀해주신 부분이라 의견 여쭤보려고 한 부분인데 잊고 있었습니다
aTob 로 한다면 nicknameMap의 경우에는 어떤 네이밍이 괜찮다고 생각하시나요?
There was a problem hiding this comment.
volunteerIdToNickname 정도로 직관적인 네이밍밖에 생각이 안나네요 ㅎㅎ 더 좋은 네이밍이 있을까요?
There was a problem hiding this comment.
volunteerIdToNickname 좋은거 같습니다! 적용시키겠습니다 ㅎㅎ
|
|
||
| @RequiredArgsConstructor | ||
| @Repository | ||
| public class VolunteerRankingRedisRepository { |
There was a problem hiding this comment.
인터페이스를 cacheRepo로 명명하고 redisRepo를 구현체로 두는 것은 어떨까요?
There was a problem hiding this comment.
말씀해주신 부분이 저희 컨벤션에 맞는거 같아요
곧 수정하겠습니다!
| Volunteer volunteer = Volunteer.createDefault(NAVER, "oauth-id-" + i); | ||
| volunteer.updateVolunteerStats(i * 10, i); | ||
| volunteerRepository.save(volunteer); |
There was a problem hiding this comment.
감사합니다 ㅎㅎ;;; 이상한 부분이 있었네요
| @DisplayName("전체, 월, 주간 랭킹 하나라도 빈 값이 반환되면 빈 리스트를 반환한다.") | ||
| @Test | ||
| void getRankingsWhenDataNotFound() { | ||
|
|
||
| // given | ||
| when(valueOperations.get("volunteer:total_ranking")).thenReturn(null); | ||
| when(valueOperations.get("volunteer:monthly_ranking")).thenReturn(null); | ||
| when(valueOperations.get("volunteer:weekly_ranking")).thenReturn(null); | ||
|
|
||
| // when | ||
| Optional<VolunteerRankingResponseDto> retrievedRankings = volunteerRankingRedisRepository.getRankings(); | ||
|
|
||
| // then | ||
| assertFalse(retrievedRankings.isPresent()); | ||
| } |
| @DisplayName("유틸리티 클래스 인스턴스화 시도 시 예외 발생") | ||
| @Test | ||
| void utilityClassCannotBeInstantiated() throws Exception { | ||
| // given | ||
| Constructor<VolunteerRankingMapper> constructor = VolunteerRankingMapper.class.getDeclaredConstructor(); | ||
| constructor.setAccessible(true); | ||
|
|
||
| // when & then | ||
| InvocationTargetException exception = assertThrows( | ||
| InvocationTargetException.class, | ||
| constructor::newInstance | ||
| ); | ||
|
|
||
| Throwable cause = exception.getCause(); | ||
| assertThat(cause).isInstanceOf(UnsupportedOperationException.class); | ||
| assertThat(cause.getMessage()).isEqualTo("유틸리티 클래스는 인스턴스화할 수 없습니다."); | ||
| } |
| @Builder | ||
| public record VolunteerRankingResponseDto( | ||
| List<VolunteerTotalRankingResponseDto> volunteerTotalRankingResponse, | ||
| List<VolunteerMonthlyRankingResponseDto> volunteerMonthlyResponse, |
There was a problem hiding this comment.
얘만 Ranking이 빠진 거 같은데 혹시 다른 의도가 있으신건가용?
There was a problem hiding this comment.
아닙니다! 그냥... 바빴나봐요 찾아주셔서 감사합니다 ㅎㅎ
|
@m-a-king Dto로 나눴던 이유를 설명드리면 |
정확히는 위 이미지에 대한 질문입니다. |
- 컨벤션에 맞는 변수명, 클래스명 수정 - 변수, 클래스명 수정에 따른 테스트 수정
|
|
@m-a-king 말씀해주신 부분을 개발할 때도 고민을 했었습니다! 변하게 된다면 한번에 변하게 될거라는 생각은 저도 같습니다! 이런식으로 구성이 되었습니다 충분히 괜찮다고 생각했지만 결론적으로 재중님과 완전 같은 의견이지만 각 필드의 쓰임이 명확한 코드가 가장 큰 이유인 것 같습니다 |
📌 과제 설명
전체, 월, 주간 봉사시간 랭킹을 반환하는 기능을 구현했습니다.
매시 50분마다 랭킹을 계산하고 Redis에 해당값을 캐싱하여
캐싱된 값을 반환하도록 구현했습니다.
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
QueryDSL의 경우 MySQL의 고급 기능 사용에 제약이 있어 불가피 하게 native query로
랭킹 조회 쿼리를 구현했습니다.
DB 전환후 발견된 문제로 컨트롤러 테스트시 Hikari connection을 제대로 반환하지않는 문제를 발견했습니다.
컨트롤러 테스트로 인해 테스트 시간이 증가했었다고 추정이 됩니다.
현재는 build시 컨트롤러 테스트를 제외시킨 상태이며 정확한 문제 진단후 해결해보겠습니다.
MySQL로의 변경, 테스트의 증가등으로 스레드 문제가 발생했던 일부 테스트 코드를 수정하였습니다.