-
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
refactor: OfferingRepository 쿼리 최적화 #578
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.
에버 고생하셨습니다!
수정하신 코드의 양이 꽤 되는데 진짜 고생했어요.
쿼리는 전보다 더 날라 갈 수 있겠지만 많이 깔끔해진 것 같아 오히려 좋은 것 같아요. 커넥션만 잘 조절하면 될 것 같습니다.
간단한 코멘트 남겨보았어요. 확인 부탁드립니다.
추가로 동적 쿼리의 필요성도 말씀해주셨는데, 쿼리 개수를 줄일 수 있을 것 같아서 사용하면 좋을 것 같습니다. QueryDsl 쓰면 좋을 것 같은데,,, 한 번 디깅해보고 사용해보죠.
검색어도 지금은 :keyword%
으로만 사용하고 있으니, 전문 검색 인덱스도 달아보고 싶네요 ㅋㅋㅋ 가능할 진 모르겠지만.. 그 부분은 제가 해보겠습니다.
그리고 안드 workflow도 같이 실행되고 있는 것 같은데, 왜 그럴까요..ㅠㅜ 낼 함께 알아보죠~
return of(offeringsWithTitleKeyword, offeringsWithAddressKeyword); | ||
} | ||
|
||
private List<OfferingEntity> of(List<OfferingEntity> firstOffering, List<OfferingEntity> secondOffering) { |
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.
메서드명을 of
로 설정하신 이유가 있을까요? toXXX 와 같은 네이밍 말고 of로 선택하신 이유가 궁금합니다!
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.
아마 커밋 내역을 보고 달아주신 리뷰 같은데요! 위 코드는 현재 concat 메서드로 변경되었습니다!
위 메서드명을 선택했던 이유는 구구가 학습테스트 코드에 사용하신 걸 보고, 간결하고 가독성 또한 좋다고 생각해서 따라 사용해봤습니다 ㅎㅎ
result.addAll(firstOffering); | ||
result.addAll(secondOffering); |
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.
커밋 내역을 보고 달아주신 리뷰 같은데요! 위 코드 역시 현재 존재하지 않고, concat 메서드에서 정렬을 진행하고 있어요!
} | ||
|
||
@Override | ||
protected List<OfferingEntity> fetchOfferingsWithLastOffering( | ||
OfferingEntity lastOffering, String searchKeyword, Pageable pageable) { | ||
return offeringRepository.findRecentOfferingsWithKeyword(lastOffering.getId(), searchKeyword, pageable); | ||
if (searchKeyword == null) { | ||
return offeringRepository.findRecentOfferingsWithoutKeyword(lastOffering.getId(), pageable); |
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.
👍
Comparator<OfferingEntity> sortCondition = Comparator | ||
.comparing(OfferingEntity::getDiscountRate) | ||
.thenComparing(OfferingEntity::getId, Comparator.reverseOrder()); | ||
return concatOfferings(pageable, sortCondition, | ||
offeringRepository.findHighDiscountOfferingsWithTitleKeyword( | ||
lastDiscountRate, | ||
lastId, | ||
searchKeyword, | ||
pageable), | ||
offeringRepository.findHighDiscountOfferingsWithMeetingAddressKeyword( | ||
lastDiscountRate, | ||
lastId, | ||
searchKeyword, | ||
pageable)); |
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.
오~ sorting이 있었군요 👍
Comparator<OfferingEntity> sortCondition = Comparator | ||
.comparing(OfferingEntity::getMeetingDate) | ||
.thenComparing(OfferingEntity::getId, Comparator.reverseOrder()); |
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.
Comparator는 다른 클래스로 분리하면 어떨까요? 이 메서드가 과한 책임을 갖고 있다고 생각합니다.
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.
넵! 메서드 추출해보았습니다
Comparator<OfferingEntity> sortCondition = Comparator | ||
.comparing(OfferingEntity::getDiscountRate) | ||
.thenComparing(OfferingEntity::getId, Comparator.reverseOrder()); |
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.
이 부분도요! 다른 객체로 분리하면 어떨까요?
SELECT o | ||
FROM OfferingEntity o | ||
WHERE o.id < :lastId | ||
AND (o.title LIKE :keyword% OR o.meetingAddress LIKE :keyword%) | ||
ORDER BY o.id DESC | ||
""") | ||
List<OfferingEntity> findRecentOfferingsWithKeyword(Long lastId, String keyword, Pageable pageable); | ||
|
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.
이유
한방 쿼리를 여러 개의 쿼리로 분리함으로써 쿼리의 조건절 중복 발생
검색어의 유무에 따라 특정 조건절(o.title LIKE :keyword% OR o.meetingAddress LIKE :keyword%)의 존재 여부가 결정됨
라이브러리 종류QueryDSL 혹은 JOOQ 도입 고려
동적쿼리 구성하는 데에 매우 찬성입니다. 다만, JPA에서 동적 쿼리를 작성할 때는 JPA Specification이라는 옵션도 고려해볼 수 있습니다. QueryDSL이나 JOOQ 같은 라이브러리를 도입하려는 이유로 동적 쿼리 작성 외에 다른 추가적인 장점이 있는지 고민해보면 좋겠습니다:)
관련해 과거 리뷰어와 나눴던 대화 공유합니다! JPA Specification
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.
고생하셨습니다. minor한 코멘트 하나 남겼습니다. 크게 중요하지 않으니 나중에 진행하셔도 좋을 것 같아요!
return concat(pageable, sortCondition(), offeringsSearchedByTitle, offeringsSearchedByMeetingAddress); | ||
} | ||
|
||
private Comparator<OfferingEntity> sortCondition() { |
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.
이 부분이 다른 클래스와 중복되어서 구현되어 있네요. 지금은 큰 문제 없지만, 나중에 Comparator용 클래스를 새로 파서 추상화하면 좋을 것 같습니다.
* refactor: 최근순 공모 조회 쿼리 최적화 * test: 테스트 날짜 변경 * refactor: 마감임박만 공모 조회 쿼리 최적화 * refactor: 높은할인율순 공모 조회 쿼리 최적화 * refactor: 참여가능만 공모 조회 쿼리 최적화 * refactor: 스케쥴러 쿼리 최적화 * refactor: 최근순 공모 조회 쿼리 최적화 * refactor: 마감임박만 공모 조회 쿼리 최적화 * refactor: 높은할인율순 공모 조회 쿼리 최적화 * refactor: 참여가능만 공모 조회 쿼리 최적화 * refactor: 쿼리 메서드 이름 컨벤션 통일 * refactor: offerings 합치는 로직 추출 * refactor: offering fetch 로직 메서드명 축약 * refactor: Strategy 별 구조 통일 * style: 파라미터 코드 개행 통일 * refactor: sortCondition 생성 메서드 추출
📌 관련 이슈
close #540
✨ 작업 내용
이번 작업 중 변경된 사안이 많은 편이라 관련하여 정리해둡니다.
🗂️ 쿼리 최적화 및 인덱스 설정 관련
노션 링크 첨부합니다.
https://silent-apparatus-578.notion.site/Offering-dddae682105e4c35b8aa157dba1d338b?pvs=4
♻️ 도메인 코드 리팩터링
기존 전략 패턴을 유지하면서 코드의 가독성을 높이기 위해 코드를 일부 수정했습니다.
1️⃣ outOfRangeId() 접근 제어자 변경
OfferingFetchStrategy의 모든 자식 클래스에서 호출하는 로직이므로 중복이라 판단
-> fetchWithoutLast의 파라미터로 전달하도록 코드 수정
-> 외부 호출이 불필요해져 접근 제어자 변경 (protected -> private)
AS-IS
TO-BE
2️⃣ OfferingFetchStrategy 메서드명 축약
OfferingRepository의 메서드가 전체적으로 많은 파라미터를 받고 있기 때문에 개행이 많아져 가독성 저하
-> 해당 메서드를 사용하는 OfferingFetchStrategy 메서드명의 불필요한 어구를 줄임으로써 가독성 향상
AS-IS
TO-BE
3️⃣ List 합치는 메서드 생성
AS-IS
한방쿼리 사용하였으므로 존재하지 않았던 메서드
TO-BE
쿼리 최적화를 목적으로 쿼리 분리
-> 조회 쿼리의 결과인 List 타입의 데이터 합치는 로직 필요
-> 아래 메서드를 OfferingFetchStrategy에 선언하여 자식 클래스에서 사용
⭐️ [중요] 제안 ⭐️
OfferingRepository에 한해 동적 쿼리 생성 라이브러리 도입을 제안합니다.
이유
o.title LIKE :keyword% OR o.meetingAddress LIKE :keyword%
)의 존재 여부가 결정됨라이브러리 종류
진행 계획
📚 기타
git merge develop
깃 브랜치 전략이 변경됨에 따라, 아래 명령어를 통해 머지 후 PR 생성하였습니다.