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

Refactor/#634: Bookmark 동시성 이슈 해결 및 이벤트 발행 #640

Open
wants to merge 2 commits into
base: refactor/dependency
Choose a base branch
from

Conversation

cpot5620
Copy link
Collaborator

작업 대상

#634
#639

📄 작업 내용

JPQL을 통한 Bookmark 동시성 이슈 해결
이벤트 발행을 통한 의존성 분리(?)

🙋🏻 주의 사항

코드 관련해서는 크게 어려운 부분은 없는 것 같은데요.
코드 외적(?)으로 이야기 해보아야할 사항이 있는 것 같아요.

1. 이벤트 관련 객체들의 패키지 위치
현재 Bookmark 추가, 삭제에 대한 이벤트 객체는 topic/domain 패키지에 두었습니다.
조금 어색해보일 수 있는데, 이는 네이밍 때문에 발생하는 문제라고 생각해요 !

또한, 지금 topic 하위 패키지에 listener 패키지를 두었는데요.
사실, TopicCommandService에 추가하려다가 이후에 더 많은 이벤트 발행이 발생하게 되면 해당 서비스 클래스가 너무 커질 것 같아서 별도로 분리했습니다.

위와 같은 패키지 구조에 대해서 어떻게 생각하십니까 !?

스크린샷

📎 관련 이슈

레퍼런스

Comment on lines +1 to +4
package com.mapbefine.mapbefine.topic.domain;

public record BookmarkAdditionEvent(Long topicId) {
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

기술 스택으로 Java17을 선택한 만큼, 레코드를 사용했는데요.
이를 도메인 패키지에 두었는데 너무 DTO 같아 보이려나요 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

아니요!

보니까 PinUpdateEvent 도 Record 네용 ㅎㅎ

오히려 일관성을 위해서 더욱이 Record 로 두어야 할 것 같습니다.

Comment on lines +36 to +38
@Modifying(clearAutomatically = true, flushAutomatically = true)
@Query("update Topic t set t.bookmarkCount = t.bookmarkCount + 1 where t.id = :topicId")
void increaseBookmarkCountById(@Param("topicId") Long topicId);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

왜 이 녀석만 flushAutomatically 옵션을 두었는지 궁금해하실 것 같아요 ㅎㅎ

이거 진짜 팀 블로그에 쓸건데... 최대한 간략하게 설명드리자면 다음과 같아요.
이전에 discussion에서, JPQL로 작성된 메서드를 호출하면 영속성 컨텍스트가 flush된다고 말씀드렸는데요.
이때 flush되는 대상들이 해당 JPQL을 수행하는 객체와 관련된 내용만 flush가 된다고 합니다.
그래서, Bookmark 저장 -> 이벤트 발행 -> Topic JPQL 수행의 과정으로 로직이 수행된다면..
clearAutomatically 옵션에 의해서 Bookmark 저장과 관련하여 쓰기 지연 저장소에 대기중인 쿼리가 삭제되버리게 되는 것이죠..
그래서 위 옵션을 통해 영속성 컨텍스트에 있는 모든 것(?)들을 flush 해주는 옵션이 생겨나게 되었답니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Topic JPQL 이 수행될 때, 자기 것만 flush 하고, 또한 clearAutomatically 옵션으로 인해서, 영속성 컨텍스트가 비워져 Bookmark 저장 쿼리가 사라진다는 의미인가요?? 제가 이해한 게 맞나요?

@cpot5620 cpot5620 added BE 백엔드 관련 이슈 우선순위 : 중 refactor 리팩토링 관련 이슈 labels Dec 25, 2023
@cpot5620 cpot5620 requested a review from kpeel5839 December 25, 2023 17:02
Copy link
Collaborator

@kpeel5839 kpeel5839 left a comment

Choose a reason for hiding this comment

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

Topic -> Bookmark 로 가는 의존성이 많이 끊어져서 굉장히 좋은 것 같아요!

그리고 listener 패키지로 따로 구분해 놓은 것도 굉장히 좋은 것 같습니다.

다만, 개인적인 의견으로는 BookmarkAdditionEvent, BookmarkDeleteEventListener 와 같은 패키지에 두어, 같이 사용되고 있음을 명확히 나타내고, 다른 패키지와의 의존성도 생기지 않는 것이 좋아보여요!

라고 할려고 했는데, 이 논리면 Service 와 Domain 객체들도 모두 같은 패키지에 놓아야 하네요.

좋은 것 같아요! 굳굳

아 마지막으로 현재 PinUpdateEvent 는 pin -> event 패키지에 있는데, 이도 Domain 으로 옮겨야 하는 것일까요?

아니면 현재 BookmarkAdditionEvent, BookmarkDeleteEvent 를 Event 라는 패키지를 만들어 옮겨야 할까요?

@jiwonh423
Copy link
Collaborator

@cpot5620 답변 기다리는데 언제 달아주시죠? 🙏 🙏

@cpot5620
Copy link
Collaborator Author

이거 진짜 언제하지..
다 까먹어서 어디부터 손 대야할지 모르겠네..

@jiwonh423
Copy link
Collaborator

이거 진짜 언제하지.. 다 까먹어서 어디부터 손 대야할지 모르겠네..

👀

@kpeel5839
Copy link
Collaborator

가자!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 관련 이슈 refactor 리팩토링 관련 이슈 우선순위 : 중
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants