-
Notifications
You must be signed in to change notification settings - Fork 0
[FIX] 유저(내) 피드 조회 시 총 피드 개수 반환 #389
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
Conversation
GiJungPark
left a comment
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.
리뷰 확인 부탁드려요!
Kim-TaeUk
left a comment
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.
👍
sansan20535
left a comment
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.
@GiJungPark 엇 제가 리뷰가 안 보이는데.. 혹시 어떤 건지 알 수 있을까요?!
GiJungPark
left a comment
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 int countVisibleFeeds(User owner, Long lastFeedId, Boolean isVisible, | ||
| Boolean isUnVisible, List<Genre> genres, | ||
| Long visitorId) { | ||
| return jpaQueryFactory | ||
| .selectFrom(feed) | ||
| .leftJoin(novel).on(feed.novelId.eq(novel.novelId)) | ||
| .leftJoin(novelGenre).on(novel.eq(novelGenre.novel)) | ||
| .leftJoin(genre).on(novelGenre.genre.eq(genre)) | ||
| .where( | ||
| feed.user.eq(owner), | ||
| ltFeedId(lastFeedId), | ||
| checkVisible(visitorId), | ||
| checkPublic(isVisible, isUnVisible), | ||
| checkGenres(genres) | ||
| ) | ||
| .fetch() | ||
| .size(); |
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; fetch 이후 size로 List의 개수를 조회하는건, 성능적으로 좋지 않을 것 같아요!
이처럼 COUNT(*) 조회하는게 좋을 것 같습니다 :)
@Override
public Long countVisibleFeeds(User owner, Long lastFeedId, Boolean isVisible,
Boolean isUnVisible, List<Genre> genres,
Long visitorId) {
return jpaQueryFactory
.select(feed.count())
.from(feed)
.leftJoin(novel).on(feed.novelId.eq(novel.novelId))
.leftJoin(novelGenre).on(novel.eq(novelGenre.novel))
.leftJoin(genre).on(novelGenre.genre.eq(genre))
.where(
feed.user.eq(owner),
ltFeedId(lastFeedId),
checkVisible(visitorId),
checkPublic(isVisible, isUnVisible),
checkGenres(genres)
)
.fetchOne();
}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.
아 .count()가 있었군요! 저도 쿼리문에서 Java의 size()를 사용하는 것이 올바른 방법인지 의문이 들었었는데 해당 방법을 사용하면 좋을 거 같아요! 반영하겠습니다~ 감사합니다 : )
리뷰 남겨주신 것에서 한 가지 더 궁금한 점은 성능적으로 정말 크게 차이가 나는지가 궁금해졌어요! DB를 한 번 더 거치는 게 아니라 단순 .size()메서드는 런타임에서 크게 시간 소요가 들지 않다고 생각이 들긴 했는데 기중님이 성능에서 좋지 않을 것 같다고 생각하신 이유가 궁금합니다 : )
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.
size() 메서드의 연산은 빠르기 때문에 별 문제가 없습니다!
문제는 조건에 맞는 Feed가 10,000개라면, 10,000개의 Feed 데이터가 DB에서 서버로 전송 된다는 점이라고 생각해요
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.
오 놓쳤는데 굿이네요
네트워크 I/O, 메모리 사용 👍
Related Issue
Key Changes
To Reviewers
References