위키 상세 조회와 댓글 조회를 분리한다 #229
Conversation
Test Results118 tests 117 ✅ 5s ⏱️ Results for commit 234d1ce. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
이상 안녕하세요. 아톰입니다. 고생 많으셨습니다!
당장 월요일에 기능 배포해야하고, 팀 리소스가 한정적이기 때문에 빠르게 반영은 어려울 것 같아요. 🥲
변경 분에 대해서 아래와 같이 정리했는데 맞을까요?
- 답변 조회 API 엔드 포인트를 분리하고 페이지네이션이 적용됐습니다.
- 답변을 조회할때 좋아요 정보 수집을 transform-> Set 형식에서 서브 쿼리 형태로 변경됐습니다.
각 작업 사항에 대해서 개인적인 의견을 작성했어요. 시간 나실 때 확인 부탁 드립니다. 🙇🏻♂️
| private QCommentSummary projectionCommentSummary(Long memberId) { | ||
| return new QCommentSummary( | ||
| comment.id, | ||
| comment.answer, | ||
| comment.isAnonymous, | ||
| memberId != null | ||
| ? JPAExpressions.selectOne() | ||
| .from(commentLike) | ||
| .where( | ||
| commentLike.comment.eq(comment), | ||
| commentLike.member.id.eq(memberId) | ||
| ) | ||
| .exists() | ||
| : Expressions.constant(false), | ||
| JPAExpressions.select(commentLike.id.count()) | ||
| .from(commentLike) | ||
| .where(commentLike.comment.eq(comment)), | ||
| comment.createdAt, | ||
| set(commentLike.member.id), | ||
| new QMemberThumbnail(member.id, member.name, member.profileImageUrl, member.githubUrl) | ||
| ); | ||
| } |
There was a problem hiding this comment.
이전에 이야기했던 것처럼 기존 transform 로직은 성능 이슈가 발생할 수 있다는 가설이 있어요.
단, 이 가설은 카티션 프로덕트의 결과가 메모리에 적재된 다음에 그루핑 및 중복 제거 작업을 수행한다는 가정이 있어요.
한 개의 위키에 답변이 1000개 있으며, 각 답변에는 좋아요가 20개씩 달려 있다고 예시를 들어볼게요.
CommentLike leftJoin 과정에서 comment + comment_like 레코드가 결합되어 20000개의 답변이 메모리에 적재되고, 이후에 가공을 수행하면 성능적인 이슈가 발생할 수 있습니다.
해당 변경 분이 위 이슈를 해결하는 건지 궁금해요. 그렇다면, JPA에서 distinct가 자동으로 추가되는지? 실제로 메모리에 적재하는지?와 같이 가설을 뒷받침하는 게 필요해 보여요. 최근 GC 관련해서 면밀히 관찰하고 있는 부분이 있다는 점에서 이상이 제안해주신 방법에 흥미를 느끼고 있습니다.
이상은 어떤 측면에서 이러한 방식이 나을 것이라고 판단하셨는지 궁금해요.
There was a problem hiding this comment.
서브쿼리는 두 가지를 질의합니다.
- 해당 댓글을 좋아요 눌렀는지?
- 해당 댓글에 좋아요가 몇 개인지?
위 방법은 조인을 하지 않고 서브 쿼리를 한 것이기 때문에 일대다 조인에서 발생하는 중복 row, 카티션 프로덕트 문제가 발생하지 않습니다. 그래서 카티션 프로덕트를 실제로 메모리 적재하는지, jpa distinct를 사용해야 하는지 생각하지 않았어요.
그리고 댓글이 1,000개 있다고 가정할 때, 서브쿼리가 1,000 번 나가는거 아닌가? 하는 생각이 들 수 있는데, limit을 통해서 제한을 두어 전체 조회하지 않도록 했습니다.
반면에 댓글 좋아요 수를 질의하는 서브 쿼리는 쿼리하는 size 만큼 무조건 발생하게 되는데, 댓글 테이블에 좋아요 수를 비정규화 컬럼으로 두고 댓글 조회 시 해당 컬럼값을 가져와서 카운트 쿼리를 하지 않을 수 있겠네요. 다만 댓글에 좋아요를 추가하거나 삭제할 때 count를 업데이트 해줘야 하고, 동시성 문제가 발생할 수 있으니 고민이 필요하겠습니다.
| @Transactional(readOnly = true) | ||
| public CommentResponses list(MemberIdentity identity, Long wikiId, Long cursorId, Long size) { | ||
| List<CommentSummary> commentSummaries = commentRepository.queryByWikiIdAndIdGreaterThan(wikiId, identity.id(), cursorId, size); | ||
| Long totalCount = commentRepository.countAllByWikiId(wikiId); | ||
|
|
||
| return CommentResponses.of(commentSummaries, totalCount); | ||
| } |
There was a problem hiding this comment.
확실히 성능이 올라갈 걸로 기대해요. 좋은 제안해주셔서 감사합니다. 👍
다만, 현재 상황에서 저는 기존 구성(페이지네이션 없음, 위키 단건 조회 API에 통합)을 선호해요. 근거는 다음과 같습니다.
- 클라이언트 편의성을 위해 한 가지 API로 통합하고 페이지네이션을 수행하지 않는다.
- 기획이 어떻게 변경될지 모른다.
- 사용자가 위키 단건을 조회해서 볼 때 다른 사용자가 남긴 답변이 보이는게 방해될 수 있다. 그러면, 답변이 토글 형태로 표시하는 식으로 기획이 발전할 수 있지 않은가? 토글이면 comment id 목록만 내려줘서 성능 최적화도 가능할 것 같다. 그러면, 페이지네이션 없어도 문제 없지 않은가? -> ID로 답변 단건 조회
- 내가 작성한 답변은 가장 상단에 노출해야한다면?
다들 어떻게 생각하시나요? 제가 놓치고 있는게 있다면, 편히 말씀 주세요. 🙇🏻♂️
There was a problem hiding this comment.
피그마에 위키 기획을 보니, 위키 글과 그 아래 댓글 리스트가 나열되는 형태를 띄고 있네요.
클라이언트 편의성을 위해 한 가지 API로 통합하고 페이지네이션을 수행하지 않는다.
기획이 어떻게 변경될지 모른다.
기존 코드는 위키와 댓글을 하나의 API 응답으로 내려주고 있고, 저는 이 PR에서 그 둘을 분리하는 코드를 작성했어요. 지금 그럴 일은 없겠지만, 댓글이 무수히 달린다면, 편의성을 위해 위키와 댓글 리스트를 하나의 응답으로 내려주던 구조에서는 즉각 문제가 발생하겠죠. 그럼 그 순간부터 편의성을 유지할 수 없게 되는데, 저는 이런 fragile한 편의라면 가짜 편의성이 아닌가 생각합니다.(ㅋㅋㅋ)
그리고 기획이 어떻게 변경될지 모르기 때문에, 더욱 그 둘을 분리하는 게 유리하지 않을까 하는 의견입니다 🤔
이 관점에서 기존에 API 호출을 한 번 하던 것에서, 위키 조회 API와 댓글 리스트 조회 API를 각각 한 번씩 호출하는 게 클라이언트 편의성에 큰 문제가 될까 의문도 드네요 😃
내가 작성한 답변은 가장 상단에 노출해야 한다면?
엄청 러프하게 생각한 거라 빈틈이 있을 것 같긴 한데,
- 내가 작성한 댓글이 있다면 조회하는 쿼리,
- 내가 작성하지 않은 다른 사람이 해당 위키에 작성한 댓글 리스트 조회 쿼리,
둘을 따로 호출하면 해결된다고 생각합니다.
There was a problem hiding this comment.
인사이트 공유해주셔서 감사합니다. 많이 배웠어요. 😀
[엔드 포인트 분리]
말씀 주신대로 엔드 포인트를 분리하는 것에 대해서는 편의성에 크게 영향을 미치지 않겠네요. 짚어주셔서 감사합니다.
제가 많이 부족해서 기획이 어떻게 변경될지 모르니 엔드 포인트를 분리해야한다는 말은 어려워요. 추가 설명 부탁 드려도 될까요?
[페이지네이션]
편의성을 위해 위키와 댓글 리스트를 하나의 응답으로 내려주던 구조에서는 즉각 문제가 발생하겠죠. 그럼 그 순간부터 편의성을 유지할 수 없게 되는데, 저는 이런 fragile한 편의라면 가짜 편의성이 아닌가 생각합니다.
댓글 페이지네이션을 MVP에서 제외하는 것이 편의성을 향상시킨다는 점은 공감이 되신건가요?
이상은 댓글이 많이 쌓이는 상황에서 구조 변경이 요구된다는 점에서 fragile한 편의라고 표현해주신 것 같아요. fragile한 편의라는 표현은 정확하다고 생각해요. 허나 저는 일시적으로 편리해도 그 순간에 다른 중요한 일에 집중할 수 있다는 점에서 가짜 편의라고는 생각하지 않아요. 이게 가짜 편의성이라고 이야기하신 이유가 무엇인지 설명을 들을 수 있을까요?
지금은 성능 최적화가 강하게 요구되는 때가 아니기에 글로벌 캐시 계층을 도입하지 않았어요. 저는 이 의사결정이 낡아질 수 있다고 생각해요. 말씀 해주신 것처럼 사용자가 많아지거나, 답변이 많아지는 등 여러 이유로 인해 캐시가 필요할 수 있어요. 그러면 지금 인프라 구조는 fragile하니 잘못된 의사결정인가요? 제가 오해하고 있는 부분이 있거나, 비약이 있다면 가감없이 말씀 주세요. 🙇🏻♂️
There was a problem hiding this comment.
제가 많이 부족해서 기획이 어떻게 변경될지 모르니 엔드 포인트를 분리해야한다는 말은 어려워요. 추가 설명 부탁 드려도 될까요?
저도 기획적으로 잘 아는게 없고, 가정이 들어가면 끝도 없지만 예를 들어보면 인스타그램 홈에서처럼 피드 리스트가 쭉 있고 댓글은 한 뎁스 들어가서 댓글 목록을 조회하게 변경될 수도 있을 것 같아서 한 말이었어요.
댓글 페이지네이션을 MVP에서 제외하는 것이 편의성을 향상시킨다는 점은 공감이 되신건가요?
바쁘다는 핑계로 현재 팀원들이 어느 부분에 집중하고 있었는지, 작업 우선순위가 어떻게 정해져있는지 정확히 파악이 안 되어 프로젝트 이해가 떨어진 상태입니다. 그래서 솔직히 공감이 안 된 상태에서 제시한 의견이긴 합니다.
매일메일을 밑바닥부터 만든 아톰이 중요하게 생각하는 시기 적절한 타이밍에 맞는 의사결정 스타일을 잘 알고 있지만, 예시로 말씀해주신 캐시 도입만큼 빅스텝이라고 생각하지는 않았습니다.
다시 보니 의도와 달리 제가 사용한 표현이 조금 지나친 감이 있네요.🥲 지금 의사 결정이 틀렸다는 뉘앙스로 사용한 것이 아니었는데 용서 부탁드립니다🙇
There was a problem hiding this comment.
ㅋㅋㅋ 괜찮습니다. 기분이 상하거나 그러진 않았어요. 오히려, 바쁘실텐데 제안해주셔서 고마운 마음입니다. 👍
실제로 이상의 PR이 도움이 되는 건 확실하구요. 저는 몇 가지 염려되는 지점을 짚고, PR을 더욱 발전 시킨 다음에 반영하고 싶다는 생각이 커요. 추가로 그 과정에서 말씀 주신 것처럼 빅스텝의 기준은 무엇이고, 스몰스텝은 무엇인지 저희 팀만의 기준을 논의하고 싶은 생각도 있습니다. 오버엔지니어링도 문제지만, 언더엔지니어링도 문제니까요.
현 상황에서 매일위키에서 우선순위가 가장 높은 것은 매일메일 사용자를 위키로 전환하는 것과 매일위키 내부에서 커뮤니티가 활성화되는 것입니다. 이러한 상황에서 페이지네이션은 추가적인 프론트엔드나 디자인 작업이 필요해요. 물론 default 값을 설정해서 미래에 연동하자는 방식으로 진행할 수 있지만, 적어도 현재는 그런 코드는 보이지 않습니다.
이상의 제안에 설득된 부분이 많아 제 입장을 다시 정리하자면, 엔드 포인트 분리랑 쿼리 변경 제안은 오케이, 페이지네이션은 default 값을 설정해서 현재 기획을 하위호환할 수 있다면 오케이입니다.
| LocalDateTime createdDateTime, | ||
| LocalDateTime modifiedDateTime |
There was a problem hiding this comment.
어드민 페이지에 각 질문지의 생성, 수정 일자가 있으면 더 편할 것 같아서 추가해봤습니다.
|
| private BooleanExpression eqSearchParam(String searchParam) { | ||
| if (searchParam == null || searchParam.isEmpty()) { | ||
| return null; | ||
| } | ||
| return question.title.likeIgnoreCase(searchParam) | ||
| .or(question.content.likeIgnoreCase(searchParam)); | ||
| } | ||
|
|
There was a problem hiding this comment.
어드민 검색 있으면 좋을 것 같아서 넣어봤습니다 🙇🏽♂️
분리해봤습니다.
이 pr을 메인에 반영하지 않더라도, 의견을 나누고자 올려봅니다.
아톰이 말씀해주신 빠르게 구현 + 클라이언트 편의성 방향 + 최적화에 관한 고민을 듣고 지금 당장 개선해야 하는 것은 아니지만,
연관관계를 느슨한 결합으로 가져가는게 좋지 않을까? 하는 의견에서 pr 올려서 프린, 아톰과 함께 생각을 공유하고자 합니다 🙇🏽♂️
오랜만에 자바 코드를 작성하기도 하고, 위키 도메인 지식이 거의 없는 상태로 코드를 작성해서 허점이 많습니다. 참고해서 흐름만 봐주세요!