-
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
[#14] 복잡한 쿼리 최적화 & 동적 스케줄링으로 변경 #17
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.
고생하셨습니다. 남긴 코멘트 확인해서 답변 남기신 후 다시 리뷰요청해주세요~
LocalDate endedAt = request.getEndedAt(); | ||
validateStartDateIsBeforeEndDate(startedAt, endedAt); | ||
List<MonthlyOrderStatistics> monthlyStatisticsList = new ArrayList<>(); | ||
getMonthlyStatistics(startedAt, endedAt, monthlyStatisticsList); |
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.
파라미터로 넘어간 monthlyStatisticsList
에 값을 추가하지 말고, 통계결과를 List로 반환하게 만들어보세요. 파라미터의 상태를 바꾸는 함수나 메서드는 결과를 예측하기 어려울 뿐 아니라 상태를 바꾸는 사이드이펙트가 있는 메서드가 됩니다.
getMonthlyStatistics(startedAt, endedAt, monthlyStatisticsList); | |
List<MonthlyOrderStatistics> monthlyStatisticsList = getMonthlyStatistics(startedAt, endedAt); |
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.
넵 반환하는걸로 변경하겠습니다 알려주셔서 감사합니다!
IntStream.range(startedAt.getMonthValue(), endedAt.getMonthValue() + 1) | ||
.parallel() | ||
.forEach(i -> { | ||
/** | ||
* 시작일부터 종료일까지 월 별로 날짜 설정 | ||
* ex. 2024년 1월 28일 부터 2024년 3월 10일 까지 조회할 경우 | ||
* <li>2024년 1월 28일부터 1월 31일</li> | ||
* <li>2024년 2월 1일부터 2월 29일</li> | ||
* <li>2024년 3월 1일부터 3월 10일</li> | ||
*/ | ||
LocalDate starDate = (i == startedAt.getMonthValue()) ? startedAt : LocalDate.of(startedAt.getYear(), i, 1); | ||
LocalDate monthEnd = (i == endedAt.getMonthValue()) ? endedAt : getLastDayOfMonth(endedAt.getYear(), i); | ||
MonthlyStatisticsParameterVo parameterVo = new MonthlyStatisticsParameterVo(starDate, monthEnd); | ||
List<MonthlyOrderStatisticsVo> monthlyStatisticsVoList = statisticsRepository.getMonthlyOrderStatistics(parameterVo); | ||
List<MonthlyOrderStatistics> monthlyStatistics = convertToDTO(monthlyStatisticsVoList); | ||
monthlyStatisticsList.addAll(monthlyStatistics); | ||
}); |
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.
monthlyStatisticsList.addAll(monthlyStatistics);
넘어온 자료구조가 Thread-safe한 자료구조인가요? 이 메서드는 Thread safe할까요?
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.
병렬스트림 사용으로 동시에 데이터를 추가할 경우 ArrayList의 add(), addAll()메소드는 Thread safe하지 않습니다! 자바의 동시성 컬렉션 라이브러리인 CopyOnWirterArrayList
로 변경하겠습니다..!
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.
리뷰 확인 후 다시 요청해주세요~
public MonthlyOrderStatistics(MonthlyOrderStatisticsVo vo) { | ||
this.yearMonth = (vo != null) ? vo.yearMonth() : null; | ||
this.totalOrderCnt = (vo != null) ? vo.totalOrderCnt() : 0; | ||
this.totalPaymentPrice = (vo != null) ? vo.totalPaymentPrice() : BigDecimal.ZERO; | ||
this.totalCouponUseCnt = (vo != null) ? vo.totalCouponUseCnt() : 0; | ||
this.totalCouponPrice = (vo != null) ? vo.totalCouponPrice() : BigDecimal.ZERO; | ||
} |
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.
생성자 파라미터로 null이 넘어와야하는 경우가 있나요? 만약 null이 넘어온다면 오류인 상황일 것으로 보이는데, 이 경우에는 명시적으로 에러를 throw해주는게 좋습니다. 이런식으로 단언문을 사용하기도 하니 참고해주세요~
public MonthlyOrderStatistics(MonthlyOrderStatisticsVo vo) { | |
this.yearMonth = (vo != null) ? vo.yearMonth() : null; | |
this.totalOrderCnt = (vo != null) ? vo.totalOrderCnt() : 0; | |
this.totalPaymentPrice = (vo != null) ? vo.totalPaymentPrice() : BigDecimal.ZERO; | |
this.totalCouponUseCnt = (vo != null) ? vo.totalCouponUseCnt() : 0; | |
this.totalCouponPrice = (vo != null) ? vo.totalCouponPrice() : BigDecimal.ZERO; | |
} | |
public MonthlyOrderStatistics(MonthlyOrderStatisticsVo vo) { | |
Assert.notNull(vo, "생성자 파라미터는 null일 수 없습니다"); | |
... | |
} |
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.
이 부분은 제가 조회에서 데이터 가져올때 어차피 1개월 기준으로 받아오니까 List가 아닌 객체로 받으면 되지 않을까 싶었는데요!
List가 아닌 객체로 받아오니까 해당하는 기간 범위에 데이터가 없을때
에 객체가 Null이 되버리는 문제가 있어서 추가했었습니다!
그런데, List로 받아오면 애초에 null인 객체가 List에 추가되지 않아서 List로 반환하는걸로 확정 했습니다.
그래서 말씀해주신 Null 체크는 없애도록 하겠습니다! 더 좋은 코드 추천해주셔서 감사합니다~!
List<MonthlyOrderStatistics> monthlyStatisticsList = getMonthlyStatistics(startedAt, endedAt); | ||
|
||
// 월별로 정렬 | ||
monthlyStatisticsList.sort(Comparator.comparing(MonthlyOrderStatistics::getYearMonth)); |
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.
List.sort
는 원본 컬랙션의 순서를 변경합니다.
어떤 객체든지 상태를 변경하는 것은 동시성이슈나 버그발생을 일으킬 수 있기 떄문에 주의해야하며, 컬랙션은 여러 메서드에서 공유하는 경우가 많아 더더욱 주의해야 합니다. 이런 경우에는 원본을 아예 변경하지 않도록 컬랙션을 stream으로 복사하는 방식을 자주 사용하니 참고해주세요.
return monthlyStatisticsList .stream().sort(Comparator.comparing(MonthlyOrderStatistics::getYearMonth)).toList();
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.
비슷한 예로 객체 내부에 컬랙션을 가지고 있는 경우에도 getter를 만들 때 원본 리스트가 아닌 복사본을 리턴하도록 만들기도 합니다.
public List<?> getFoo() {
return List.copyOf(list);
}
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.
아하 stream을 사용하면 원본 객체를 복사해서 작업하기 때문에 동시성이슈나 버그발생
이 안일어나는군요! 알려주셔서 감사합니다..!
group by YearMonth | ||
order by YearMonth; |
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.
group by가 있더라도 MySQL은 group by한 컬럼대로 순서를 보장해주지 않죠. 명시적으로 order by를 걸어준 부분 좋습니다 👍
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.
감사합니다~ group by한 컬럼대로 순서를 보장하지 않기 때문에 order by 사용!
select DATE_FORMAT(od.created_at,'%Y-%m') as YearMonth, | ||
count(od.id) as totalOrderCnt, | ||
SUM(od.total_payment_price) as totalPaymentPrice, | ||
count(oc.id) as totalCouponUseCnt, | ||
SUM(od.total_discount_price) as total_discount_price | ||
from order_detail od | ||
left join order_coupon oc on od.id = oc.order_id | ||
where od.created_at BETWEEN #{startedAt} and #{endedAt} | ||
group by YearMonth | ||
order by YearMonth; |
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.
SQL문은 대부분 SNAKE_CASE를 사용합니다. camelCase를 사용하지 않으니 SQL문에 대해서는 파라미터명과 쿼리문에 snake case를 사용해주세요. 왜냐하면 대부분의 DB에는 대/소문자의 구분을 하지 않도록 설정하는 옵션이 있기 때문에 camelCase를 사용하면 오타가 있어도 쿼리가 실행되는 경우가 생깁니다. 이런 경우를 방지하기 위해 어지간하면 SNAKE_CASE를 사용해주세요~
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.
앗 이 부분은 계속 SNAKE_CASE를 사용하지 않고 있네요.. 습관처럼 SNAKE_CASE를 사용할 수 있도록 신경쓰겠습니다!
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.
앗 멘토님 alias에서 yearMonth를 SNAKE_CASE로 사용하면 내장 함수랑 동일한 키워드라서 SQLSyntaxErrorException
에러가 납니다..! yearMonth만 ``백틱으로 감싸겠습니다!
Quality Gate passedIssues Measures |
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.
고생하셨습니다. 머지해주셔도 좋습니다~
* feat : 월별 통계 조회 기능 * refactor : 불필요한 내용 제외 및 주석 수정 * refactor : 동적 스케줄링으로 변경 * refactor : 순서 변경 * refactor : 주석정리 * style : 코드 스타일 정리 * refactor : 인덱스 설정한 내용 추가 * refactor : 집계함수 분리 시 사용했던 VO제거 * refactor : 요청 일자 검증 예외 추가 * refactor : 요청 일자 검증 Exception 추가 * refactor : 로직 변경 - 1달 기준으로 안해도 됨 * refactor : 병렬스트림 사용 시 Thread safe 한 자료구조 사용 및 파라미터로 Collection 넘기지 말고 반환타입으로 사용하기 * refactor : MySQL 문법 SNAKE_CASE로 변경 및 원본객체를 정렬할때는 동시성이슈를 피하기 위해 stream적용 * refactor : MySQL 내장함수 Date_format은 문자열로 변환해서 포맷화하기 때문에 더 느림
쿼리 최적화 - 복합인덱스, 커버링 인덱스 사용
집계함수 별로 쿼리 분리를 시도했지만, 여전히 속도 저하 발생
Month()만 사용하거나, count함수만 사용해도 분리한
쿼리 당 4.5 ~ 4.9초 소요 되는 현상
발생따라서 분리한 쿼리들을 애플리케이션 레벨에서 반환타입에 매핑하면 16초~20초 소요 (1월부터 5월이라고 가정할 경우)
날짜 기준으로 필터링
되는거라 해당 날짜에 대한 레코드가 얼마나 쌓였는지에 따라 다릅니다.대안
월 별 기준
으로 쿼리를 날리는 방법병렬 스트림 사용
병렬 스트림
을 사용해서 속도는 6s에서 2~3s대로 50% 개선했지만, 고려해야할 점이 있습니다.