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

[BE] feat: Club 페이징 적용 및 batch size 설정 #622

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

J-I-H-O
Copy link
Contributor

@J-I-H-O J-I-H-O commented Oct 7, 2024

이슈

개발 사항

  • batch size 옵션 설정
  • 페이징 구현

전달 사항

  • 이슈에 적힌 내용 꼭 확인해주세요! 저희 의사 결정 과정을 담아두었습니다.

Copy link

github-actions bot commented Oct 7, 2024

Test Results

200 tests   200 ✅  19s ⏱️
 44 suites    0 💤
 44 files      0 ❌

Results for commit 446c230.

♻️ This comment has been updated with latest results.

Comment on lines 75 to 83
private Slice<Club> fetchClubSlice(FindClubByFilterRequest request, LocalDateTime lastFoundCreatedAt, Long lastFoundId) {
return clubRepository.findAllBy(
request.province(),
Gender.toGenders(request.genderParams()),
SizeType.toSizeTypes(request.sizeParams()),
PageRequest.ofSize(request.pageSize()),
lastFoundCreatedAt,
lastFoundId
);
Copy link
Contributor

Choose a reason for hiding this comment

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

ofSize 예정대로 20개의 2배로 잡고
hasNext를 그대로 내려주지 않고, fetch해서 result에 add했을 때, fetch -> result로 포함안된
추가적인 record가 있을 경우 저희가 hasNext를 true로 변환해서 응답해줄 수 있을 것 같습니다.
추가 구현 후 다시 리뷰받도록 하져

Comment on lines +20 to +25
club.getAllowedSizes().stream()
.map(clubSize -> clubSize.getClubSizeId().getAllowedSize())
.collect(Collectors.toSet()),
club.getAllowedGenders().stream()
.map(clubGender -> clubGender.getClubGenderId().getAllowedGender())
.collect(Collectors.toSet())
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 List -> Set으로 바꾸는거죠? 한 줄로 될 것 같아요!

Comment on lines +99 to +100
this.allowedGenders = allowedGenders.stream().map(gender -> new ClubGender(this, gender)).toList();
this.allowedSizes = allowedSizes.stream().map(sizeType -> new ClubSize(this, sizeType)).toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 stream까지 필요없을 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

map때문에 stream한 것 같은데 다른 방법이 있나요??

Copy link
Contributor

Choose a reason for hiding this comment

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

앗 맞네요 ㅋㅋ

Comment on lines +37 to +39
public boolean isSameGenderWith(Gender gender) {
return this.getClubGenderId().getAllowedGender() == gender;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public boolean isSameGenderWith(Gender gender) {
return this.getClubGenderId().getAllowedGender() == gender;
}
public boolean hasGender(Gender gender) {
return this.getClubGenderId().getAllowedGender() == gender;
}

이름 제안해봅니당

Comment on lines +37 to +39
public boolean isSameSizeWith(SizeType sizeType) {
return this.getClubSizeId().getAllowedSize() == sizeType;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public boolean isSameSizeWith(SizeType sizeType) {
return this.getClubSizeId().getAllowedSize() == sizeType;
}
public boolean hasSizeType(SizeType sizeType) {
return this.getClubSizeId().getAllowedSize() == sizeType;
}

제안

Comment on lines -218 to +203
boolean isNotFull = !this.memberCapacity.isCapacityReached(countClubMember());

return hasJoinablePet && isNotFull && !isAlreadyJoined(member) && isOpen();
return hasJoinablePet && !isAlreadyJoined(member) && isOpen();
Copy link
Contributor

Choose a reason for hiding this comment

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

isOpen에 isNotFull이 포함이군요 ~!!

Copy link
Contributor

@ehtjsv2 ehtjsv2 left a comment

Choose a reason for hiding this comment

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

고생했습니다. servie로직에서 궁금한 게 있는데, 대면으로 물어보는 게 나을 것 같아서 찾아가겠습니다~

Comment on lines +99 to +100
this.allowedGenders = allowedGenders.stream().map(gender -> new ClubGender(this, gender)).toList();
this.allowedSizes = allowedSizes.stream().map(sizeType -> new ClubSize(this, sizeType)).toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

map때문에 stream한 것 같은데 다른 방법이 있나요??

FROM ClubSize CS
WHERE C = CS.clubSizeId.club AND CS.clubSizeId.allowedSize IN :searchingSizes
)
ORDER BY C.createdAt DESC, C.id DESC
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 의문이 들었는데요,

1.

createdAt으로 먼저 정렬하고 id값으로 정렬한다는 것은 같은 createdAt이 있을 수 있다는 가정인 것이겠죠??
사실 이 정도로 정확성을 맞춰야 하는가 싶기도 하고, id는 pk이지만 정렬 기준을 두 번째로 했기 때문에, 기존에 pk로 만들어진 엔덱스는 사용할 수 없을 것으로 보입니다.
아마 fileSort가 createdAt한번 id 한번 총 두번 나올 것 같아요. 복합인덱스를 하지않았다면.

2.

id를 정렬기준에 놓았다는 것은 auto-increment라는 가정하에 한 것 같습니다. id를 정렬기준에서 하지 말아야한다는 주장은 아닙니다, 어차피 id를 정렬기준으로 놓는다면 createdAt은 정렬기준에서 없어도 되는 게 아닌 가 싶습니다. createdAt->id 정렬 하면 결국 id를 기준으로 정렬된다고 생각합니다.
제 주장은 createdAt만 정렬기준으로 두거나, id만 정렬기준으로 두거나 입니다. createdAt만 한다면 인덱스를 걸면 성능이 좋겠네요~!

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 합리적인 것 같습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

정합성을 맞춘다고 추가한 것은 맞아요. composite index를 추가하는 것이 정합성을 맞추는 공수보다 덜 든다고 생각했어요.

제 주장은 createdAt만 정렬기준으로 두거나, id만 정렬기준으로 두거나 입니다.

id 생성전략에 의존적인 형태로 개발을 하고 싶지 않았어요.
사실 저희 서비스에서 생성시점이 ms 기준으로 중복될 정도의 데이터가 현실적으로 발생하냐고 물어보면 아니라고 생각해요(슬프지만..)
다만, 실제 테스트를 해보면 그런 상황이 종종 나와요.
비록 최신순 정렬이라는 별 것 없는 기능으로 보일 수 있지만 어느 정도 저희가 제어가능하고 예상 가능한 결과값이 나와야한다는 생각이였습니다.

Copy link
Contributor

@ehtjsv2 ehtjsv2 Oct 10, 2024

Choose a reason for hiding this comment

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

id 생성전략에 의존적인 형태로 개발을 하고 싶지 않았어요.

제가 하고싶은 말이랑 같은 말입니다. 이미 id를 정렬기준에 넣은 이상 auto-increment를 가정하고있고, id의 생성전략에 의존적인 형태로 개발하고 있는 것이랑 같지 않냐는 뜻입니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

사실 저희 서비스에서 생성시점이 ms 기준으로 중복될 정도의 데이터가 현실적으로 발생하냐고 물어보면 아니라고 생각해요(슬프지만..)
다만, 실제 테스트를 해보면 그런 상황이 종종 나와요.

나온다고 해도 그정도의 정합성이 필요한 서비스인가? 라는 질문을 하고싶었던 것입니다. 저도 아예안나온다고 생각하지는 않구요!

Comment on lines 51 to 55
@GetMapping("/searching")
public ApiResponse<List<FindClubByFilterResponse>> findByFilter(
public ApiResponse<FindClubPageByFilterResponse> findByFilter(
@Auth Long memberId,
@Valid @ModelAttribute FindClubByFilterRequest request
) {
Copy link
Contributor

@takoyakimchi takoyakimchi Oct 11, 2024

Choose a reason for hiding this comment

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

이전 버전 앱 사용자는 명세가 바뀌면서 예외가 터질 수도 있을 것 같아요.
단순히 기존 명세에서 필드가 추가만 되었다고 하더라도 예측이 쉽지만은 않을 것 같아요.
버저닝 하는 건 어떨까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖥 backend backend 🤹 enhance 성능 개선 ✨ feat 기능 개발 🛠️ refactor 리팩터링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

페이징 DTO 분리 모임 리스트 조회 시 페이징 적용 batch size 전역 설정
4 participants