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

[team-31] 데이먼 3주차 1번째 PR 요청 #236

Open
wants to merge 106 commits into
base: team-31
Choose a base branch
from

Conversation

dukcode
Copy link
Collaborator

@dukcode dukcode commented Jun 29, 2022

안녕하세요 Peter 오랜만에 인사드립니다.!

이번 진행 사항은 다음과 같습니다.

진행사항

  • Issue 관련 API 구현
  • Comment, Reaction 관련 API 구현
  • 피드백 적용 리팩토링
  • 쿼리 최적화

관련 질문

관련 질문은 file에 코멘트로 달겠습니다.

feat: Issue, Milestone API 구현
* API 구현
- Admin 토큰을 헤더에 보내면 Admin으로 로그인
* Swagger 문서 업데이트
- Issue 필드에 statusChangedAt, statusChangeUser 추가
- Comment 필드에 systemMessage 필드 추가
@dukcode dukcode added the review-BE Improvements or additions to documentation label Jun 29, 2022
@dukcode dukcode self-assigned this Jun 29, 2022
String loginName = jwtUtil.getPayload(token);
System.out.println("loginName = " + loginName);
return loginName;
return userRepository.findByLoginName(loginName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Argument Resolver에서 User를 직접 검색해와서 이제 Controller에서 User를 직접 받을 수 있게 되었습니다.

따라서 서비스 레이어에서 User를 검색하는 코드를 한곳에 모을 수 있었는데

이렇게 되면 컨트롤러에서 도메인에 대한 의존성이 필연적으로 생기게 됩니다.

LINK 에서는 Service 레이어가 도메인을 캡슐화하는 계층이라고 언급하고 있는데

컨트롤러에서의 도메인 의존에 대해서 피터는 어떻게 생각하시는지 궁금합니다.

private final JPAQueryFactory queryFactory;

public List<Issue> findAllByCondition(IssueSearchCondition condition, Pageable pageable) {
return queryFactory
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

해당 코드에서는 status, author, assignedUsers, IssueLabels에 따른 검색조건으로 Issue를 DB에서 검색하고 있습니다.

  1. 검색 조건에 따른 Issue 목록 조회
  2. 페이징

두가지의 기능을 하는데 여기서 문제가 있습니다.

  1. Issue에 일대다 조건이 2개 걸려있다. (AssignedUsers, IssueLabels)
  2. 두 조건이 모두 검색조건에 포함된다.
  3. 여기서 fetch join 사용 여부에 따라 두가지 방법을 생각해 봤습니다.
  • fetch join을 사용하면 페이징을 메모리에서 해야한다.(현재 적용된 방식)

    • 데이터 많을 시 성능 문제 발생
    • MultipleBagFetchException은 Collection을 Set으로 변경하여 해결하였습니다.
  • fetch join을 사용하지 않으려면 해당 조건을 빼고 검색하고 메모리에서 stream filter를 통해 걸러낸다.

    • 해당 조건을 빼고 검색하면 검색 결과가 100개라면 메모리에서 filter을 거치면 5개가 될 수 있다.
    • default_batch_fetch_size 옵션을 켠다고 해도 모든 이슈에 대한 assignedUsers, IssueLabels 들을 불러와야 한다.
    • 따라서 검색 쿼리에 페이징 적용이 불가능하다.

따라서 결론은 일대다 검색조건을 포함한 경우에

모든 Issue에 대한 일대다 조건을 불러와야 하고 (검색 조건에 따른 페이징 때문) 메모리에서 페이징을 해야하므로 쿼리가 1개라도 덜 날라가는 첫번째 방식을 택했습니다.

성능상 좋은 방법이 있을까요?

Choose a reason for hiding this comment

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

각 방식에 대한 장단점을 잘 인지하신것 같네요! 👍
첫번째 방식은 말씀하신대로 데이터가 많아질 경우 여러테이블을 조인하면서 쿼리 자체의 성능이 좋지 못할 수 있습니다. 다만 조건으로 검색을하면 데이터 양이 많이 줄어들기 때문에 페이징을 메모리에서 하더라도 큰 문제는 없을 것으로 보입니다.

MultipleBagFetchException은 Collection을 Set으로 변경하여 해결하였습니다.

set으로 해결가능한 부분인지는 저도 모르고있었네요 ^^;
무리한 join으로 쿼리수를 줄이는게 무조건 좋은건 아니지만, 중요하지 않은 쿼리가 추가적으로날아가는건 피하는게 좋다고봅니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

갑자기 든 생각인데 조건이 존재하느냐 조건이 존재하지 않느냐에 따라서 쿼리를 나누면 더 성능이 괜찮아 질 것 같다는 생각이 드네요.. 조건이 없는 검색이 조건이 있는 검색보다 훨씬 많은 요청이 예상되므로

조건이 null이라면 일대다 조인을 할 필요 없이 batch_select로 해결할 수 있고.. 조건이 있는 경우에는 어쩔수 없이 메모리에서 페이징을 하는 방식을 적용하면 조금더 성능향상이 있을 것 같은데 피터는 어떻게 생각하시나요?

.orElseThrow(() -> new IllegalArgumentException(
"존재하지 않는 issue 입니다. issueId = " + issueId));
return new IssueDetailResponse(issue);
Issue issue = issueQueryRepository.findIssueWithAuthorAndMilestone(issueId);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue Detail 조회의 결과값으로 User(AssignedUser에 연결된)Label(IssueLabel에 연결된)을 뿌려줘야 합니다.

Issue - AssignedUser - User, Issue - IssueLabel - Label일대다 - 다대일 관계를 가지고 있습니다.

기존 방식대로 작동해보면 default_batch_fetch_size를 켜더라도 AssignedUser -> User로 갈때 IN쿼리가 발생하지 않았습니다.

아마도 default_batch_fetch_size는 검색 된 결과의 직접적인 일대다 Collection field 값에만 적용이 되는걸로 보였습니다.

따라서 IN쿼리가 나가게 하기 위해 AssignedUser 및 IssueLabel 을 검색하고 필드의 일대다 Collection에 접근했더니 그제서야 IN쿼리가 나갔습니다.

성능 최적화를 위해 해당방법을 사용하는게 적절한 방식인지 아니면 더 나은방법이 있는지 여쭤보고 싶습니다.

Choose a reason for hiding this comment

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

현재 issueDetail을 내려주기위해서 4~5개의 쿼리가 수행되는 것으로 보이네요.

  1. Issue 조회 (author, milestone 조인)
  2. assignedUsers + user 조회
  3. issueLabels + label 조회
  4. comment 조회( + reaction 조회)

issue 에 엮인 연관객체가 많기 때문에 하나의 쿼리로 불러오기보다, 현재 구현한 방식대로 불러오는게 불가피한 방식이라고 보이네요.
여러 테이블을 조인해서 하나의 쿼리로 수행한다고 무작정 좋은건 아니니 적절하게 데이터가 많은걸 우선적으로 조인하고 나머지는 lazy loading으로 불러오는 방식도 괜찮아보입니다.
즉, issue, assignedUser, issuelabel을 조회하기 위해선 3번의 쿼리가 수행되어야하는셈인 듯 합니다.

(long) issueResponses.size()
, issueResponses);
return new OpenClosedCountResult<>(openClosedCount.getOpenCount(),
openClosedCount.getClosedCount(), issueResponses);
}

@Transactional
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

delete 쿼리에도 너무 여러번 쿼리가 나가는거 같아서 질문드립니다.

  • 가장 먼저 id로 Issue select 쿼리 발생
  • Cascade.ALL인 AssignedUser select 쿼리 발생
    • 결과가 N개라면 N개의 delete쿼리 발생
  • Cascade.ALL인 Comment select 쿼리 발생
    • 결과가 N개라면 Comment와 일대다 관계를 가지고 있는 Reaction select 쿼리 N개 발생
    • Comment 가 평균 M개의 Reaction을 가지고 있다면 delete Reaction쿼리 M * N 개 발생
    • N개의 delete Comment쿼리 발생
  • Casacde.ALL인 IssueLabel select 쿼리 발생
    • 결과가 N개라면 N개의 delete쿼리 발생

질문 에서 사용한 방법처럼 Issue 로 일대다를 검색하고 BatchDelete를 사용하려 했는데 JPA 에서 순서가 코드작성한대로 나가지 않아서 Constraint문제가 발생해서 기존 코드로 원복했습니다.

더 나은 방법이 분명히 있을 것 같아 질문드립니다.

Choose a reason for hiding this comment

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

삭제의 경우 연관객체의 삭제로 인해 응답시간이 늦어진다면
issue 만 먼저 지우고 나머지 연관객체는 비동기로 지우는 방법도 고려해볼 수 있습니다.
(issue를 지우는게 이 API의 주된 목적이니까요!)

예시로 spring event listener를 사용해서 내부 메모리에 이벤트를 발행하고, 비동기로 이 이벤트를 컨슘하여 삭제처리를 수행하는 방식으로하면 issue를 delete하는 API의 응답시간을 많이 줄일 수 있을 거라고 봅니다..!
또하나의 방법은 스케줄러를 돌려서 issue(부모)가 지워진 comment, reaction을 순차적으로 지우는 함수를 실행할수도있을 것 같네요

또한 batchDelete를 사용하고 싶다면 지우는 순서를 지정해서 의도적으로 delete 함수를 실행해줄수도 있을것 같네요 :)

issue.addComment(comment);
List<AssignedUser> assignedUsers = createAssignedUsers(request.getAssigneeIds(), issue);
List<IssueLabel> issueLabels = createIssueLabels(request.getLabelIds(), issue);
assignedUserRepository.saveAll(assignedUsers);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue 생성도 처음엔 Cascade.ALL 를 사용하여 생성하려고 했지만 일대다 관계가 많아질 수록 여러번의 쿼리가 나가서 따로 생성한 후 saveALL메서드를 사용하고 batch insert옵션을 켜서 in쿼리로 나가게 했습니다.

Choose a reason for hiding this comment

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

👍

(long) filteredMilestoneResponses.size(),
filteredMilestoneResponses);
List<MilestoneResponse> milestoneResponses =
milestoneQueryRepository.findAllByStatusWithFetchIssues(status)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Milstone을 조회할 때 엮여있는 Issue 의 OPEN, CLOSED 를 카운트 하여 view에 보여줘야 합니다. 현재 방식은 모든 Issue 를 fetch join 하고 메모리에서 filter 로 카운트하여 보여주고 있는데 비효율적인 것 같아서 두가지 방법을 생각하고 있습니다.

  1. Milestone 에 issueOpenCount, issueClosedCount 를 만들고 이슈 추가 및 열고 닫힐 때마다 값을 변경
  2. count용 쿼리 새로 짜기 -> 그런데 LIst로 어떻게 담아오고 결과를 매핑할지 잘 모르겠습니다.

Choose a reason for hiding this comment

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

네 말씀하신 두 가지 방법은 마일스톤을 불러올때, issue 목록 없이 카운트만 포함하여 내려줘야할 상황이라면
모두 기존의 비효율을 커버할 수 있는 좋은 방법입니다.!
다만 마일스톤을 불러올 때 issue목록을 항상 같이 내려줘야한다면 count를 filter하는 게 크게 비효율이라고 생각하지는 않습니다.

issue목록을 꼭 내려줄 필요가 없다는 가정하에 말씀하신
첫번째 방법은 milestone에 open/close count를 필드가 갖는 의미가 나름 크다면, 필드를 따로 두고 관리하는 방법도 좋겠죠. 대체로 count(좋아요 수 등)를 내려줄 때 많이 사용하는 방법이고

두번째 방법도 count용 집계 쿼리를 사용한다면 쿼리결과를 redis를 통해 캐시할 수도 있습니다.. 다만 이때는 실제 issue의 open, close가 일어날때마다 캐시를 evict시켜주어야 겠죠!

카운트는 만약 데이터 정합성, 실시간성이 크게 중요하지 않는 상황이라면, 캐시를 많이 사용하기도 한답니다.
하지만 이슈트래커에서는 나름 데이터의 정합성이 중요해보이네요 :)

@dukcode dukcode requested a review from yeonnseok June 29, 2022 09:12
Copy link

@yeonnseok yeonnseok left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어 죄송합니다.
전체적으로 잘구현해주셨구요! 고민되시는 포인트위주로 코멘트 달았습니다! 수고하셨습니다!!

@@ -12,12 +13,13 @@

@Component
@RequiredArgsConstructor
public class LoginNameArgumentResolver implements HandlerMethodArgumentResolver {
public class LoginUserArgumentResolver implements HandlerMethodArgumentResolver {

Choose a reason for hiding this comment

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

👍

Comment on lines +86 to +87
public void validateAuthor(User user) {
if (!user.equals(getAuthor())) {

Choose a reason for hiding this comment

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

user를 전체가 비교하기보단, user의 unique한 값을 꺼내서 check하는건 어떨까요?

Milestone milestone = getMilestone(request.getMilestoneId());

issue.updateMilestone(milestone);
}

private Issue findIssueWithExistValidation(Long issueId) {

Choose a reason for hiding this comment

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

메서드명에 existValidation같은 내용은 굳이 작성하지 않아도 좋을 것 같아요~

issueLabelRepository.deleteAll(issue.getIssueLabels());

List<IssueLabel> issueLabels = createIssueLabels(request.getLabelIds());
List<IssueLabel> issueLabels = createIssueLabels(request.getLabelIds(), issue);

Choose a reason for hiding this comment

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

issueLabel과 assignedUser 꼭 모두 지우고 다시 다 넣는 방식으로 구현되어야할까요?

(long) filteredMilestoneResponses.size(),
filteredMilestoneResponses);
List<MilestoneResponse> milestoneResponses =
milestoneQueryRepository.findAllByStatusWithFetchIssues(status)

Choose a reason for hiding this comment

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

네 말씀하신 두 가지 방법은 마일스톤을 불러올때, issue 목록 없이 카운트만 포함하여 내려줘야할 상황이라면
모두 기존의 비효율을 커버할 수 있는 좋은 방법입니다.!
다만 마일스톤을 불러올 때 issue목록을 항상 같이 내려줘야한다면 count를 filter하는 게 크게 비효율이라고 생각하지는 않습니다.

issue목록을 꼭 내려줄 필요가 없다는 가정하에 말씀하신
첫번째 방법은 milestone에 open/close count를 필드가 갖는 의미가 나름 크다면, 필드를 따로 두고 관리하는 방법도 좋겠죠. 대체로 count(좋아요 수 등)를 내려줄 때 많이 사용하는 방법이고

두번째 방법도 count용 집계 쿼리를 사용한다면 쿼리결과를 redis를 통해 캐시할 수도 있습니다.. 다만 이때는 실제 issue의 open, close가 일어날때마다 캐시를 evict시켜주어야 겠죠!

카운트는 만약 데이터 정합성, 실시간성이 크게 중요하지 않는 상황이라면, 캐시를 많이 사용하기도 한답니다.
하지만 이슈트래커에서는 나름 데이터의 정합성이 중요해보이네요 :)

@@ -80,7 +81,7 @@ public void deleteIssue(Long issueId) {
}

@Override
public IssueCreateResponse createIssue(IssueCreateRequest request, String loginName) {
public IssueCreateResponse createIssue(IssueCreateRequest request, User loginName) {

Choose a reason for hiding this comment

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

파라미터 이름 한번 확인해주세요~

private final JPAQueryFactory queryFactory;

public List<Issue> findAllByCondition(IssueSearchCondition condition, Pageable pageable) {
return queryFactory

Choose a reason for hiding this comment

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

각 방식에 대한 장단점을 잘 인지하신것 같네요! 👍
첫번째 방식은 말씀하신대로 데이터가 많아질 경우 여러테이블을 조인하면서 쿼리 자체의 성능이 좋지 못할 수 있습니다. 다만 조건으로 검색을하면 데이터 양이 많이 줄어들기 때문에 페이징을 메모리에서 하더라도 큰 문제는 없을 것으로 보입니다.

MultipleBagFetchException은 Collection을 Set으로 변경하여 해결하였습니다.

set으로 해결가능한 부분인지는 저도 모르고있었네요 ^^;
무리한 join으로 쿼리수를 줄이는게 무조건 좋은건 아니지만, 중요하지 않은 쿼리가 추가적으로날아가는건 피하는게 좋다고봅니다

(long) issueResponses.size()
, issueResponses);
return new OpenClosedCountResult<>(openClosedCount.getOpenCount(),
openClosedCount.getClosedCount(), issueResponses);
}

@Transactional

Choose a reason for hiding this comment

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

삭제의 경우 연관객체의 삭제로 인해 응답시간이 늦어진다면
issue 만 먼저 지우고 나머지 연관객체는 비동기로 지우는 방법도 고려해볼 수 있습니다.
(issue를 지우는게 이 API의 주된 목적이니까요!)

예시로 spring event listener를 사용해서 내부 메모리에 이벤트를 발행하고, 비동기로 이 이벤트를 컨슘하여 삭제처리를 수행하는 방식으로하면 issue를 delete하는 API의 응답시간을 많이 줄일 수 있을 거라고 봅니다..!
또하나의 방법은 스케줄러를 돌려서 issue(부모)가 지워진 comment, reaction을 순차적으로 지우는 함수를 실행할수도있을 것 같네요

또한 batchDelete를 사용하고 싶다면 지우는 순서를 지정해서 의도적으로 delete 함수를 실행해줄수도 있을것 같네요 :)

issue.addComment(comment);
List<AssignedUser> assignedUsers = createAssignedUsers(request.getAssigneeIds(), issue);
List<IssueLabel> issueLabels = createIssueLabels(request.getLabelIds(), issue);
assignedUserRepository.saveAll(assignedUsers);

Choose a reason for hiding this comment

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

👍

.orElseThrow(() -> new IllegalArgumentException(
"존재하지 않는 issue 입니다. issueId = " + issueId));
return new IssueDetailResponse(issue);
Issue issue = issueQueryRepository.findIssueWithAuthorAndMilestone(issueId);

Choose a reason for hiding this comment

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

현재 issueDetail을 내려주기위해서 4~5개의 쿼리가 수행되는 것으로 보이네요.

  1. Issue 조회 (author, milestone 조인)
  2. assignedUsers + user 조회
  3. issueLabels + label 조회
  4. comment 조회( + reaction 조회)

issue 에 엮인 연관객체가 많기 때문에 하나의 쿼리로 불러오기보다, 현재 구현한 방식대로 불러오는게 불가피한 방식이라고 보이네요.
여러 테이블을 조인해서 하나의 쿼리로 수행한다고 무작정 좋은건 아니니 적절하게 데이터가 많은걸 우선적으로 조인하고 나머지는 lazy loading으로 불러오는 방식도 괜찮아보입니다.
즉, issue, assignedUser, issuelabel을 조회하기 위해선 3번의 쿼리가 수행되어야하는셈인 듯 합니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants