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

쿼리 추가 발생 문제 해결 #798

Merged
merged 9 commits into from
Nov 25, 2024
Merged

쿼리 추가 발생 문제 해결 #798

merged 9 commits into from
Nov 25, 2024

Conversation

ay-eonii
Copy link
Contributor

PR의 목적이 무엇인가요?

  • 목록 조회 시 목록 수 만큼 쿼리가 추가로 발생하는 문제를 해결했습니다.

이슈 ID는 무엇인가요?

설명

  • 모임 목록 조회 API에서 목록 수(N)에 따라 추가적으로 N개의 쿼리를 발생시킵니다.

    • 이를 IN 연산자를 사용해 한번에 조회하도록 변경하고
    • implements 레이어에서 동일한 moimId로 매핑하도록 변경했습니다.
    • MoimFinder의 createMoimOverview
      private List<MoimOverview> createMoimOverview(List<Moim> moims, List<ChamyoMoim> chamyoMoims,
      	Set<Long> zzimedMoimIds) {
      	Map<Long, Long> chamyoMap = chamyoMoims.stream()
      		.collect(Collectors.toMap(ChamyoMoim::getMoimId, ChamyoMoim::getChamyoCount));
    
      	return moims.stream()
      		.map(moim -> new MoimOverview(
      			moim,
      			chamyoMap.getOrDefault(moim.getId(), 0L),
      			zzimedMoimIds.contains(moim.getId())
      		))
      		.toList();
      }
  • @ManyToOne의 default인 EAGER 설정으로 불필요한 쿼리가 발생하는 것을 확인했습니다.

    • 찜한 모임 목록 조회에서 DarakbangMember
    • 나의 모임 목록 조회에서 DarakbangMember
    • zzim과 darakbangMember를 지연로딩으로 변경했습니다.

질문 혹은 공유 사항 (Optional)

목록 조회 쿼리 개선 블로그에 성능 비교를 참고해주세용

@ay-eonii ay-eonii changed the title Refactor/#797 쿼리 추가 발생 문제 해결 Nov 21, 2024
Copy link
Contributor

@Mingyum-Kim Mingyum-Kim left a comment

Choose a reason for hiding this comment

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

안녕하세요 에이아니 ~
덕분에 모임 목록 조회 API가 엄청 빨라졌네요 👍👍
Very Cool 합니다. 컨플릭트 발생 예상되는 부분이 있어서 RC 남겨요 같이 합쳐봐요 😋

Comment on lines 45 to 49
List<Moim> moims = moimRepository.findAllByDarakbangIdOrderByIdDesc(darakbangId);
List<ChamyoMoim> chamyoMoims = chamyoRepository.findAllByMoims(moims);
Set<Long> zzimedMoimIds = zzimRepository.findAllByDarakbangMemberId(darakbangMember.getId());

return createMoimOverview(moims, chamyoMoims, zzimedMoimIds);
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분은 제가 기존에 해결한 부분이 있어서 (머지는 안됨) 컨플릭트가 발생할 것 같은데요 🤔🤔
어떻게 합칠 지 이야기해봐요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞아용 ...ㅠㅅㅠ 이 부분은 할까말까 하다가 찜한모임목록, 나의모임목록 조회 시 동일한 메서드(findAll..., createMoimOverview)를 쓸 수 있길래 일단 수정한 상태입니닷

안나가 해결한 방식과 거의 동일해서 안나거 먼저 머지하고 충돌해결해도 괜찮을 거 같은데 어떠신가여 ?

@@ -25,6 +26,7 @@
@Table(name = "moim")
@Getter
@NoArgsConstructor
@EqualsAndHashCode
Copy link
Contributor

Choose a reason for hiding this comment

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

요건 id를 기준으로 동등성 비교를 하나요?
아님 모든 값으로 동등성 비교하나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

모든 값으로 비교합니당 제외하고 싶은 필드나 지정하고 싶은 필드가 있으면 exclude, of 에 설정하면 됩니닷

Copy link
Contributor

@pricelees pricelees left a comment

Choose a reason for hiding this comment

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

재택 기간인데 고생이 많으시네요 !
비록 캠퍼스에 나가진 못하고있지만..ㅎㅎ 먼 곳에서 항상 응원하고 있습니닷
고생하셨어요 ~~ (Approve 준다고는 안했습니다 ~)

Comment on lines 45 to 47
List<Moim> moims = moimRepository.findAllByDarakbangIdOrderByIdDesc(darakbangId);
List<ChamyoMoim> chamyoMoims = chamyoRepository.findAllByMoims(moims);
Set<Long> zzimedMoimIds = zzimRepository.findAllByDarakbangMemberId(darakbangMember.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

이게 노션에 남겨주신 방법 1인 것 같네요 ~ 개인적으로는 모임이 '찜'을 아는게 크게 문제가 되는 일인가 싶고, 이 과정에서 메서드 흐름이 더 복잡하게 느껴지는 것 같은데요 ! 저는 join을 쓰는 2번이 더 좋은 방법 같습니닷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

흑흑 하지만 이건 재사용할 수 있어요 흑흑

Copy link
Contributor

Choose a reason for hiding this comment

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

흑흑 하지만 이건 재사용할 수 있어요 흑흑

이미 답이 정해졌구나.. ㅋㅋㅋㅋㅋㅋ😡😡

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋ 더 열심히 설득하면 반영할 의향 있어요 😋

Comment on lines 54 to 56
Set<Long> moimIds = chamyoMoims.stream()
.map(ChamyoMoim::getMoimId)
.collect(Collectors.toSet());
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 내 모임을 조회할 때도 순서대로 정렬하는게 있었던 것 같은데요 ~! Set으로 했을 때 순서가 보장이 될까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

여기는 순서 보장이 필요없어서 Set으로 했습니당! response는 아래에 List 순서에 따라 매핑합니닷!

Comment on lines 86 to 87
Map<Long, Long> chamyoMap = chamyoMoims.stream()
.collect(Collectors.toMap(ChamyoMoim::getMoimId, ChamyoMoim::getChamyoCount));
Copy link
Contributor

Choose a reason for hiding this comment

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

바로 아래에서는 '현재 참여중인 인원'을 currentPeople로 표현했는데 여기서는 chamyoCount로 표현하고 있네요 ~ ㅎㅎ 둘중 하나로 통일하면 좋겠습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헤헤 넹 !!

return moims.stream()
.map(moim -> new MoimOverview(
moim,
chamyoMap.getOrDefault(moim.getId(), 0L),
Copy link
Contributor

Choose a reason for hiding this comment

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

여기까지의 흐름을 보면,

  1. 찜으로 모임을 조회
  2. 모임으로 참여를 조회(List -> List

의 흐름인데, 그러면 여기서의 moim에 대한 chamyoMap의 count는 반드시 존재해야 하는 값인 것 같아요 ~ 이게 없으면 0으로 넣을게 아니라 오류가 나는게 조금 더 타당한 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어랍쇼 ?? 그렇네요 !! 왜 저는 그 생각을 못했을까요 ㅋㅋ 감사합니다!!


그런데 목록이다 보니 하나 오류났다고 전체 조회를 못하는 건 곤란할 거 같아요. 참여인원이 없다면 제외하고 예외는 던지지 않도록 하겠습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

어랍쇼 ?? 그렇네요 !! 왜 저는 그 생각을 못했을까요 ㅋㅋ 감사합니다!!

그런데 목록이다 보니 하나 오류났다고 전체 조회를 못하는 건 곤란할 거 같아요. 참여인원이 없다면 제외하고 예외는 던지지 않도록 하겠습니다!

좋은 방법이네요~~ 😄

GROUP BY c.moim
ORDER BY c.moim.id DESC
""")
List<ChamyoMoim> findAllByDarakbangMemberId(long darakbangMemberId);
Copy link
Contributor

Choose a reason for hiding this comment

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

ChamyoMoim을 보면 dto 패키지에 정의했던데, 레포에서 DTO를 꺼내는건 기존 컨벤션과 어긋나지 않나요? 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

음 이 dto는 insfrastructure <-> service 범위에서 사용되길래 괜찮지 않을까 했습니다 🤔 그런데 상돌말도 일리가 있네요
중복코드를 제거하고 불필요한 쿼리를 줄이다가 여기서 moimId와 currentPeople을 담은 클래스를 반환해야 가장 깔끔해서 해당 방법을 선택했어요 . 다른 방법은 잘 .. 모르겠네요 ㅠ

혹시 domain 패키지에 ChamyoMoim을 넣으면 좀 컨벤션이 맞춰질까요 ?ㅎ

Copy link
Contributor

Choose a reason for hiding this comment

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

음 이 dto는 insfrastructure <-> service 범위에서 사용되길래 괜찮지 않을까 했습니다 🤔 그런데 상돌말도 일리가 있네요 중복코드를 제거하고 불필요한 쿼리를 줄이다가 여기서 moimId와 currentPeople을 담은 클래스를 반환해야 가장 깔끔해서 해당 방법을 선택했어요 . 다른 방법은 잘 .. 모르겠네요 ㅠ

혹시 domain 패키지에 ChamyoMoim을 넣으면 좀 컨벤션이 맞춰질까요 ?ㅎ

기존 코드를 보면 레이어와 상관없이 DTO를 반환하는게 없었어서요~ 한번 고민해 보시죠!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ㅎㅎ 일단 눈속임으로 domain에 넣었어요 ^^,,

Comment on lines 19 to 27

@Query("""
SELECT
z.moim.id
FROM Zzim z
WHERE z.darakbangMember.id = :darakbangMemberId
ORDER BY z.id DESC
""")
Set<Long> findAllByDarakbangMemberId(long darakbangMemberId);
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 헷갈릴 수 있는 여지가 있겠는데요 ~ ZzimRepository에서의 findAl이면 일반적으로는 Collection을 기대할 것 같아요 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

나이스 눈썰미 👀

@Getter
@EqualsAndHashCode
@AllArgsConstructor
public class ChamyoMoim {
Copy link
Contributor

Choose a reason for hiding this comment

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

이름이 안이뻐요 ㅋㅋ😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@pricelees pricelees 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 45 to 47
List<Moim> moims = moimRepository.findAllByDarakbangIdOrderByIdDesc(darakbangId);
List<ChamyoMoim> chamyoMoims = chamyoRepository.findAllByMoims(moims);
Set<Long> zzimedMoimIds = zzimRepository.findAllByDarakbangMemberId(darakbangMember.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

흑흑 하지만 이건 재사용할 수 있어요 흑흑

이미 답이 정해졌구나.. ㅋㅋㅋㅋㅋㅋ😡😡

return moims.stream()
.map(moim -> new MoimOverview(
moim,
chamyoMap.getOrDefault(moim.getId(), 0L),
Copy link
Contributor

Choose a reason for hiding this comment

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

어랍쇼 ?? 그렇네요 !! 왜 저는 그 생각을 못했을까요 ㅋㅋ 감사합니다!!

그런데 목록이다 보니 하나 오류났다고 전체 조회를 못하는 건 곤란할 거 같아요. 참여인원이 없다면 제외하고 예외는 던지지 않도록 하겠습니다!

좋은 방법이네요~~ 😄

GROUP BY c.moim
ORDER BY c.moim.id DESC
""")
List<ChamyoMoim> findAllByDarakbangMemberId(long darakbangMemberId);
Copy link
Contributor

Choose a reason for hiding this comment

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

음 이 dto는 insfrastructure <-> service 범위에서 사용되길래 괜찮지 않을까 했습니다 🤔 그런데 상돌말도 일리가 있네요 중복코드를 제거하고 불필요한 쿼리를 줄이다가 여기서 moimId와 currentPeople을 담은 클래스를 반환해야 가장 깔끔해서 해당 방법을 선택했어요 . 다른 방법은 잘 .. 모르겠네요 ㅠ

혹시 domain 패키지에 ChamyoMoim을 넣으면 좀 컨벤션이 맞춰질까요 ?ㅎ

기존 코드를 보면 레이어와 상관없이 DTO를 반환하는게 없었어서요~ 한번 고민해 보시죠!

Copy link
Contributor

@Mingyum-Kim Mingyum-Kim left a comment

Choose a reason for hiding this comment

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

멋지다 김테니 ~

@ay-eonii ay-eonii merged commit e77f98a into develop-backend Nov 25, 2024
1 check passed
@ay-eonii ay-eonii deleted the refactor/#797 branch November 25, 2024 06:44
@ay-eonii ay-eonii self-assigned this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants