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

[#33] 피드를 여행일지로 묶기 #34

Merged
merged 70 commits into from
Feb 11, 2024
Merged

[#33] 피드를 여행일지로 묶기 #34

merged 70 commits into from
Feb 11, 2024

Conversation

jzakka
Copy link
Collaborator

@jzakka jzakka commented Feb 8, 2024

여행일지의 정보

Field Type Null Key Default Extra
ID bigint NO PRI NULL auto_increment
MEMBER_ID bigint NO MUL NULL
TITLE varchar(50) NO NULL
THUMBNAIL_PATH varchar(255) NO UNI NULL
  • 섬네일은 피드 이미지중에서 고를 수 있다.
  • 여행일지는 Day1, Day2,.. 여행일자로 구성되고 그 안에서 피드를 참조하는 형식
  • 사용자의 여행일지를 조회하면 간략한 정보만 제공(일지id, 섬네일 경로, 제목)

@jzakka jzakka added the enhancement New feature or request label Feb 8, 2024
@jzakka jzakka requested a review from f-lab-moony February 8, 2024 06:36
@jzakka jzakka self-assigned this Feb 8, 2024
- Add:여행일지 생성 예외 클래스
- Add:여행일지 조회 예외 클래스
Copy link
Collaborator

@f-lab-moony f-lab-moony left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다! 피드백 확인 부탁드려요 ~

private String title;

@Column(name = "THUMBNAIL_PATH", unique = true, nullable = false)
private String thumbnailPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

피드 이미지중에 고르는거면 GraphicContent의 id를 넣어주는게 좋지 않을까요 ~? path 관리포인트가 분산되는 것 같아요

}

@DeleteMapping("/diaries/{diaryId}")
public ResponseEntity<Object> cancelDiary(@PathVariable Long diaryId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거 응답 그냥 void로 해도 될거같아요

}

@GetMapping("/diaries/member/{memberId}")
public Page<SimpleDiaryResponse> getMemberDiaries(@PathVariable Long memberId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Page가 응답 그대로 나가는건 좋지 않을 것 같아요. 클라이언트까지 프레임워크에 종속성이 생기는거라서, 페이징 방식만 바꿔도 클라이언트 수정이 필요하게 될 것 같습니다 ~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

�Page를 사용하면 현재 페이지, 다음 페이지 여부 같은걸 제공해줘서 좋다고 생각했는데 클라이언트에 종속성을 만드는 건 좀 그렇긴 하네요. 🥲

Copy link
Collaborator

Choose a reason for hiding this comment

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

사실 그래서 응답 DTO를 따로 만들어도 페이지 관련 정보를 넣게 되긴 할거에요..! 근데 그건 저희가 이 응답만을 위해 만드는 클래스라 독립적으로 사용할수가 있어서 클래스가 많아지더라도 각각의 DTO를 만드는게 좋습니다!

Diary savedDiary = diaryRepository.save(diary);

List<FeedResponse> feedResponses = feeds.stream()
.map(feed -> FeedResponse.from(feed, likeRepository.getLikes(feed.getId().toString())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

요것두 쿼리 여러번 날아갈 것 같아요 ~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

likeRepository는 redis에만 연결돼있어서 여러번 접속해도 db만큼 부담되지는 않을 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 요거 레디스군요 ..
구조상 바로 여러개의 좋아요 수를 가져오긴 조금 힘들것 같네요
하지만 건당 호출은 DB라서 부담된다기 보다는 외부 api 호출시엔 네트워크 통신을 여러 번 하게 되는 점 + 서버 측에서 여러 개의 요청을 처리하면서 생기는 부하 ( 스레드 증가라던지 ) 가 문제가 되는거라, 이런 경우엔 가능한 묶어서 호출할 수 있도록 하는게 좋아요

레디스 파이프라인을 쓰면 개선이 가능하지 않을까 싶은데, 지금은 그냥 두고 나중에 성능테스트 하면서 개선점으로 한번 적용해보면 좋은 경험이 되지 않을까 싶네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

'서버측에서 여러개의 요청을 처리하면서 생기는 부하'라는 게 DB에 쿼리를 날릴 때마다 커넥션이 여러개 생겨서 그에 따른 부하가 맞나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

DB만 그런건 아니고, 근본적으론 통신을 위한 파이프라인이 여러개 생기는 것에 따른 부하가 맞습니다. 근데 여기서 파생될 수 있는 부하들이 종류가 많아요. 예를 들면 서버가 동기방식으로 구성되어있는 경우 각 요청에 스레드가 할당되면서 생기는 문제라던지 .. 작업에 따라서 요청량이 많은게 메모리 문제를 일으킬 수도 있고, DB같은 경우는 여기서 File IO도 일어나니까 눈에 띄는 병목지점이 되는거구요 기본적으로 외부 통신은 여러건이 호출되면 다 부하라고 보시면 됩니다. 그래서 여러개의 응답이 필요한경우 단건호출이 되면 병목지점이 될 가능성이 높아요.

지금같은 경우도 저희가 받는 요청 한건이 n건의 요청으로 파생되기때문에 레디스가 받는 부하량은 상상이상이 될 수도 있어서 가능한 조심하는게 좋아요!

return new PageImpl<>(diaryPage.map(SimpleDiaryResponse::from).toList(), diaryPage.getPageable(), diaryPage.getTotalElements());
}

@PostAuthorize("returnObject.memberId() == authentication.principal.id")
Copy link
Collaborator

Choose a reason for hiding this comment

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

슬랙으로 말씀 드리긴 했지만 요거 책임을 도메인이 가져가는게 좋을 것 같아요 ~

Copy link

Copy link
Collaborator

@f-lab-moony f-lab-moony left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다!
피드백 좀 남겨뒀는데 확인 부탁드리고 요것도 머지 해둘게요 ~

Comment comment = commentRepository.findById(Objects.requireNonNull(commentId))
.orElseThrow(CommentSearchException::new);

comment.delete();
return CommentResponse.from(comment);
}

@Transactional
public CommentResponse deleteCommentIfOwner(Long commentId, Long memberId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
방향은 잘 맞은 것 같네요!
하지만 요 검증로직 ( 변경 시 멤버 확인 )은 entity로 들어가는게 맞을 것 같아요..!
도메인에서 처리한다 라고 하면 Entity 클래스에서 처리한다 라고 이해해주시면 될 것 같습니다 ~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 다음 PR올릴때 수정해두겠습니다

@f-lab-moony f-lab-moony merged commit b2353e7 into main Feb 11, 2024
2 checks passed
@jzakka jzakka deleted the feature/33 branch February 11, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants