-
Notifications
You must be signed in to change notification settings - Fork 4
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
발생하는 쿼리 확인 후 쿼리 성능 개선 #758
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.
코드적으로 성능적으로나 많은 개선이 이루어져서 너무 좋아요 👍
몇가지 궁금한점이 있어서 질문 남겨보았어요!
쿼리분석과 쿼리개선의 긴 여정 고생많으셨습니다~!!
private long score; | ||
|
||
@Builder | ||
private MemberMetric(final Member member, final long postCount, final long voteCount, final long score) { |
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.
개행이 빠져있어요~
return maybeMember.orElseGet(() -> { | ||
final Member savedMember = memberRepository.save(member); | ||
final MemberMetric memberMetric = MemberMetric.builder() | ||
.member(member) | ||
.postCount(0) | ||
.voteCount(0) | ||
.build(); |
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.
Q.
score는 따로 기본값으로 지정안하는 이유가 따로 있나요??
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 List<PostCommentCountDto> getCommentCountsInPosts(final Set<Long> postIds) { | ||
return jpaQueryFactory.select( | ||
new QPostCommentCountDto( | ||
post.id, | ||
comment.count() | ||
) | ||
) | ||
.from(post) | ||
.innerJoin(comment).on(comment.post.eq(post)) | ||
.where(post.id.in(postIds)) | ||
.groupBy(post.id) | ||
.fetch(); | ||
} |
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.
Q.
해당 메서드를 PostRepository가 아닌querydsl의 PostCustomRepositoryImpl에서 만드신 이유가 궁금합니다!
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.
응답으로 커스텀 DTO
를 받고 있기 때문에 querydsl
로 구현하는 것이 더 가독성 측면에서 좋다고 판단하였습니다 !
@Formula("(select count(*) from comment c where c.post_id = id)") | ||
private long commentCount; | ||
|
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.
Q.
혹쉬 commentCount도 voteCount처럼 post 테이블 컬럼에 추가되면 성능측면에서 좋아질수 있는 고려대상에 속하나요??
단순한 궁금증입니다!
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.
댓글 수 조회로 인해 성능에 문제가 발생하는 시점에 수정해보는 것도 좋은 경험이 될 것 같아요 :)
final List<PostOption> postOptions = Stream.of(originPostOptionId, newPostOptionId) | ||
.sorted() | ||
.map(postOptionRepository::findByIdForUpdate) | ||
.map(postOption -> postOption.orElseThrow( | ||
() -> new NotFoundException(PostOptionExceptionType.NOT_FOUND))) | ||
.toList(); | ||
|
||
final PostOption originPostOption = getPostOptionById(postOptions, originPostOptionId); | ||
final PostOption newPostOption = getPostOptionById(postOptions, newPostOptionId); |
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.
Q.
선택지옵션의 id가 선택지 옵션의 순번을 보장했었나요?? squence 필드가 있어야지고 헷갈리네요
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
순으로 동작하도록 구현하였기에 현재는 순번을 보장하고 있습니다 !
현재는 sequence
가 사용하지 않은 필드가 되어버린 것 같네요 😂
추후에 드래그를 통해서 옵션 순서를 수정할 수 있는 기능이 생긴다면 sequnce
필드가 유용하게 쓰일 것이라 생각해요 :)
/** | ||
* 게시글 락을 걸지 않으면 투표 2개 생기는 문제 발생 | ||
*/ | ||
@Test | ||
@DisplayName("동시에 투표를 하더라도 투표는 1번만 되어야 한다.") | ||
void concurrentlyOnlyVote() throws Exception { | ||
// given | ||
ExecutorService executorService = Executors.newFixedThreadPool(2); | ||
CountDownLatch latch = new CountDownLatch(2); | ||
AtomicInteger successCount = new AtomicInteger(); | ||
AtomicInteger failCount = new AtomicInteger(); | ||
|
||
Member member = memberTestPersister.builder().save(); | ||
memberMetricTestPersister.builder().member(member).save(); | ||
Post post = postTestPersister.postBuilder().save(); | ||
PostOption postOptionA = postTestPersister.postOptionBuilder().post(post).sequence(1).save(); | ||
PostOption postOptionB = postTestPersister.postOptionBuilder().post(post).sequence(2).save(); | ||
post.addPostOption(postOptionA); | ||
post.addPostOption(postOptionB); | ||
|
||
// when | ||
executorService.submit(() -> { | ||
try { | ||
voteService.vote(member, post.getId(), postOptionA.getId()); | ||
successCount.incrementAndGet(); | ||
} catch (Exception e) { | ||
System.out.println("EXCEPTION 1 : " + e.getMessage()); | ||
failCount.incrementAndGet(); | ||
} finally { | ||
latch.countDown(); | ||
} | ||
}); | ||
executorService.submit(() -> { | ||
try { | ||
voteService.vote(member, post.getId(), postOptionB.getId()); | ||
successCount.incrementAndGet(); | ||
} catch (Exception e) { | ||
System.out.println("EXCEPTION 2 : " + e.getMessage()); | ||
failCount.incrementAndGet(); | ||
} finally { | ||
latch.countDown(); | ||
} | ||
}); | ||
latch.await(); | ||
|
||
System.out.println("SUCCESS : " + successCount); | ||
System.out.println("FAIL : " + failCount); | ||
|
||
// then | ||
assertSoftly(softly -> { | ||
softly.assertThat(postRepository.findById(post.getId()).get().getVoteCount()).isEqualTo(1); | ||
softly.assertThat(memberMetricRepository.findByMember(member).get().getVoteCount()).isEqualTo(1); | ||
}); | ||
} |
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.
세상 모든 기능을 테스트하는 그날까지 🦅
@Test | ||
@DisplayName("동시에 투표를 수정하더라도 투표 수가 일치한다.") | ||
void concurrentlyChangeVote() throws Exception { | ||
// given | ||
ExecutorService executorService = Executors.newFixedThreadPool(2); | ||
CountDownLatch latch = new CountDownLatch(2); | ||
|
||
Member memberA = memberTestPersister.builder().save(); | ||
Member memberB = memberTestPersister.builder().save(); | ||
memberMetricTestPersister.builder().member(memberA).save(); | ||
memberMetricTestPersister.builder().member(memberB).save(); | ||
Post post = postTestPersister.postBuilder().save(); | ||
PostOption postOptionA = postTestPersister.postOptionBuilder().post(post).sequence(1).save(); | ||
PostOption postOptionB = postTestPersister.postOptionBuilder().post(post).sequence(2).save(); | ||
post.addPostOption(postOptionA); | ||
post.addPostOption(postOptionB); | ||
voteService.vote(memberA, post.getId(), postOptionA.getId()); | ||
voteService.vote(memberB, post.getId(), postOptionB.getId()); | ||
|
||
// when | ||
executorService.submit(() -> { | ||
try { | ||
voteService.changeVote(memberA, post.getId(), postOptionA.getId(), postOptionB.getId()); | ||
} finally { | ||
latch.countDown(); | ||
} | ||
}); | ||
executorService.submit(() -> { | ||
try { | ||
voteService.changeVote(memberB, post.getId(), postOptionB.getId(), postOptionA.getId()); | ||
} finally { | ||
latch.countDown(); | ||
} | ||
}); | ||
latch.await(); | ||
|
||
// then | ||
assertSoftly(softly -> { | ||
softly.assertThat(postRepository.findById(post.getId()).get().getVoteCount()).isEqualTo(2); | ||
softly.assertThat(postOptionRepository.findById(postOptionA.getId()).get().getVoteCount()).isEqualTo(1); | ||
softly.assertThat(postOptionRepository.findById(postOptionB.getId()).get().getVoteCount()).isEqualTo(1); | ||
softly.assertThat(memberMetricRepository.findByMember(memberA).get().getVoteCount()).isEqualTo(1); | ||
softly.assertThat(memberMetricRepository.findByMember(memberB).get().getVoteCount()).isEqualTo(1); | ||
}); | ||
} |
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.
Q.
궁금한점이 있습니다!
정렬을 주석처리하고 테스트 결과 돌려보니 한 선택지는 투표개수가 2개 다른 선택지의 투표개수는 0개 이렇게 결과가 나오더라고요
저는 서로 원하는 자원이 상대방에 할당되어 있어 두 프로세스가 무한정 wait
상태에 빠짐을 데드락 상태로 이해하고 있어서
실행결과가 어떤 데드락과 관련한 예외 메시지가 뜰 줄 알았는데 위와 같은 결과 나오더라고요!
어떤 상황이여서 저런 결과가 나오는건가요??
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.
저도 그 부분이 의아한 부분이긴 했습니다 🤔
콘솔 창에 deadlock
을 검색해보시면 deadlock
관련 에러가 발생한 것은 확인할 수 있었는데, 무한정 대기 상태에는 빠지지 않더라구요. 아마 일정 시간 wait
후 데드락임을 감지하고 걸려있던 락을 포기함으로 남은 1개의 옵션 개수만 처리됨으로 한쪽 옵션에만 2개의 투표가 남아있는 상황이 발생한 것으로 이해했습니다 !
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.
저와 페어하시느라 수고 많으셨습니다 다즐
2가지 리뷰 남겼습니다 :)
역시 ㅡ 즐 ㅡ
} | ||
} |
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.
P1
개행이 필요해 보여요!
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.
꼼꼼한 리뷰 감사합니다 ㅎㅎ
@Query( | ||
nativeQuery = true, | ||
value = "select" + | ||
" case" + | ||
" when YEAR(CURRENT_DATE) - m.birth_year < 10 then 0" + | ||
" when YEAR(CURRENT_DATE) - m.birth_year < 20 then 1" + | ||
" when YEAR(CURRENT_DATE) - m.birth_year < 30 then 2" + | ||
" when YEAR(CURRENT_DATE) - m.birth_year < 40 then 3" + | ||
" when YEAR(CURRENT_DATE) - m.birth_year < 50 then 4" + | ||
" when YEAR(CURRENT_DATE) - m.birth_year < 60 then 5" + | ||
" else 6 end as ageGroup," + | ||
" m.gender as gender," + | ||
" count(m.id) as voteCount" + | ||
" from vote as v" + | ||
" left join member as m on v.member_id = m.id" + | ||
" left join post_option as p on v.post_option_id = p.id" + | ||
" where p.post_id = :post_id" + | ||
" group by" + | ||
" ageGroup, m.gender" + | ||
" order by" + | ||
" ageGroup, m.gender desc" |
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.
이 부분은 사실 이번 기능과 전혀 상관없는 부분이긴 하지만,
이 정도로 긴 쿼리라면 JPQL String으로 적는 것보다 QueryDsl로 구현하는 것이 더 안전하지 않을까 하는 생각이 드네요.
가독성 측면에서도 queryDsl이 더 좋을 것 같기도 하구요.
어떻게 생각하시는지 궁금합니다.
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.
답변감사합니다!
고생하셨어요~
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.
뭔가 배우는 내용이 많았던 pr인 것 같네요.
레전드 테스트에 감탄하고 갑니다!
return switch (postSortType) { | ||
case LATEST -> post.id.desc(); | ||
case HOT -> post.voteCount.desc(); | ||
default -> OrderByNull.DEFAULT; |
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.
Q
IDE를 사용하면서 생긴 궁금증인데 default가 회색 글씨로 나오더라구요.
default에 접근하는 경우가 존재할 수 있는거죠..?
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.
PostSortType
이 Null
인 경우 default
가 적용되게 됩니다 🤓
@@ -12,4 +15,7 @@ public interface PostOptionRepository extends JpaRepository<PostOption, Long> { | |||
@Query("delete from PostOption p where p.post.id = :postId") | |||
void deleteAllWithPostIdInBatch(@Param("postId") final Long postId); | |||
|
|||
@Lock(LockModeType.PESSIMISTIC_WRITE) | |||
@Query("SELECT po FROM PostOption po where po.id = :postOptionId") | |||
Optional<PostOption> findByIdForUpdate(@Param("postOptionId") final Long postOptionId); |
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.
Q
비관적 락을 사용하신 부분에는 ForUpdate
라는 suffix를 붙이신걸로 이해하면 될까요?
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.
맞습니다 :)
@@ -87,22 +89,19 @@ public void validateToken(final String token) { | |||
.parseClaimsJws(token); | |||
} catch (final UnsupportedJwtException e) { | |||
log.info("지원하지 않는 JWT입니다."); | |||
throw new IllegalArgumentException("지원하지 않는 JWT입니다."); | |||
throw new BadRequestException(AuthExceptionType.UNSUPPORTED_TOKEN); |
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.
👍🏻🙇🏻♂️
* 게시글 락을 걸지 않으면 투표 2개 생기는 문제 발생 | ||
*/ | ||
@Test | ||
@DisplayName("동시에 투표를 하더라도 투표는 1번만 되어야 한다.") |
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.
진짜 테스트는 레전드네요
레전드 학습하고 갑니다~
# Conflicts: # backend/src/main/java/com/votogether/domain/member/service/MemberService.java # backend/src/main/java/com/votogether/domain/post/service/PostCommandService.java # backend/src/main/java/com/votogether/domain/post/service/PostQueryService.java
🔥 연관 이슈
📝 작업 요약
발생하는 쿼리 확인 후 쿼리 성능 개선을 진행하였습니다.
⏰ 소요 시간
12시간!
모든 API를 확인하고 발생하는 쿼리 정리 후 문서화하는 과정에서 많은 시간이 소요되었습니다.
🔎 작업 상세 설명
post
,post_option
테이블에vote_count
컬럼을 두어 투표 수에 대한 역정규화를 진행하였습니다.AI
전략을 사용하고 있기에created_at
이 아닌id
로 정렬을 수행하여 최신순 조회에 대한 성능을 개선하였습니다.post
테이블의vote_count
컬럼에 인덱스를 설정하여 인기순 조회에 대한 성능을 개선하였습니다.member_metric
테이블을 생성하였습니다.member_metric
테이블의score
컬럼에 인덱스를 설정하여 상위 10명의 랭킹 조회 성능을 개선하였습니다.🌟 논의 사항
아래의 문서에 최대한 자세히 설명할 수 있도록 정리해보았습니다! 읽어보시고 설명이 필요한 부분이나 질문 있으시다면 언제든지 환영입니다 :)
https://husky-dodo-c5e.notion.site/4-4c118fd7b7874646892797115f3bd64b?pvs=4