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

DeleteEventListener 내부 로직 비동기 처리 #748

Closed
wants to merge 3 commits into from

Conversation

jjongwa
Copy link
Member

@jjongwa jjongwa commented Nov 6, 2023

📄 Summary

AsyncDeleteProcessor 라는걸 만들었습니다.. 네이밍은 잘 모르겠어서 지선생님께 맡겼습니다..
이렇게 말고 각 서비스에 delete 메서드를 만들고 @Async 를 붙이는 방법도 생각해 봤는데,
그것보단 AsyncDeleteProcessor라는 클래스를 만들어 관리하는게 조금 더 보기 편하지 않을까 라는 생각이 들어서
이렇게 진행해 봤습니다.
질문 반박 뭐든 만관부

🙋🏻 More

close #747

Copy link

github-actions bot commented Nov 6, 2023

📝 Jacoco Test Coverage

Total Project Coverage 79.03%
File Coverage [91.07%]
DeleteEventListener.java 100% 🍏
TransactionalDeleteProcessor.java 82.14% 🍏

Copy link
Collaborator

@LJW25 LJW25 left a comment

Choose a reason for hiding this comment

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

이런건 어디서 배워오셨죠 디노

Comment on lines 51 to 52
asyncDeleteProcessor.deleteDayLogs(dayLogIds);
asyncDeleteProcessor.deleteItems(itemIds);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Item에 DayLogId ForeignKey 걸려있는데 데이로그 먼저 삭제해도 되나요?! 헷갈려서 물어봄

Copy link
Member Author

Choose a reason for hiding this comment

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

Soft Delete는 무적입니다

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

Choose a reason for hiding this comment

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

와우 soft Delete를 생각안했네 엄청나군요..

Copy link
Collaborator

@hgo641 hgo641 left a comment

Choose a reason for hiding this comment

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

디 노 짱


@Async
@Transactional(propagation = REQUIRES_NEW)
@TransactionalEventListener(fallbackExecution = true)
@TransactionalEventListener
Copy link
Collaborator

Choose a reason for hiding this comment

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

fallbackExecution=true 왜 지우셨나용?

Copy link
Member Author

Choose a reason for hiding this comment

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

저번에 실험하다가 붙여놓고 까먹었습니다..
대충 요약하면
image
이런데 그냥 디폴트 해놔도 될듯...!

Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 메서드에 @Transactional이 붙어있으니 무조건 트랜잭션이 있는 상태에서 이벤트를 처리하므로 상관이 없다는 뜻 맞죠? 👍


@Component
@RequiredArgsConstructor
public class AsyncDeleteProcessor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

DeleteEventListener 내부에서 로직을 비동기 메서드로 나눌수도 있었을텐데, delete와 관련된 비동기 작업을 수행하는 ÀsyncDeleteProcessor를 만드신 이유가 뭔가요? 다른 곳에서도 활용될 가능성이 있다고 판단하신건가요? 궁금합니다!

삭제라는 공통점만 있는 비동기 작업들을 AsyncDeleteProcessor로 관리했을 때 단점은 없을까요? 저희는 도메인 컨텍스트를 위주로 패키지를 분리해왔는데 삭제라는 공통점으로 이 친구들을 하나의 클래스로 묶었을 때 이슈는 없을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. 어싱크 요놈은 내부 메서드에서 못붙입니다.. 그래서 이런 짓을 해버림

  2. 날카로운 지적입니다.
    그래서 다른 방법이 각각의 서비스에다 메서드를 만든 다음 어싱크 붙여서 해당 메서드를 리스너에서 호출하는 건데,
    위에 말한대로 가독성 측면에서 별로인거 같구,
    서비스에 갑자기 어싱크 하나 띡 달려있는 메서드 있으면 이상한거 같기도 하고 그래서 고민입니다...
    그리고 또 다른 고민은 이렇게 안하면 리스너에서 Repository랑 Service 모두 의존하고 있는 형식이라(사실 지금도 이름만 다르지 같은 상황이긴 함) 그것도 좀 걸리긴 했어요..

Copy link
Collaborator

Choose a reason for hiding this comment

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

굉장한 고민을 하셨군요 디노! ! ! 역시 리더 디노입니다!!! 더 좋은 구조를 고민해보겠습니다! ! !


@Component
@RequiredArgsConstructor
public class AsyncDeleteProcessor {
@Transactional(propagation = Propagation.REQUIRES_NEW)
public class TransactionalDeleteProcessor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

특정한 로직을 transactional하게 delete해준다는 invoker느낌이 강해지는 것 같네요! 이젠 얘한테 void타입의 delete로직을 수행하는 함수형인터페이스를 넘겨줘서 실행해도 괜찮겠다는 생각이 ......... 들었습니다...

Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 느낌일까요?

    public void invoke(final Long memberId, final DeleteInvoker invoker) {
        tripDeleteInvoker.delete(memberId, tripRepository);
    }
  • 함수 호출시
transactionalDeleteProcessor
     .invoke(
           event.getMemberId(),
          (Long memberId, TripRepository repository) -> repository.deleteByMemberId(memberId)
);

Copy link
Collaborator

@hgo641 hgo641 Nov 23, 2023

Choose a reason for hiding this comment

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

굉장합니다! invoke의 인자로 익명클래스만 받아도 될 것 같습니다! memberId라는 인자에 국한되지 않아도 될 것 같습니다. AsyncDeleteProcessor or TransactionalDeleteProcessor라는 클래스를 만들거라면 이 방식도 괜찮아보입니다. 물론 전 Delete가 집중하지 말고 각 도메인별로 이벤트리스너만들자 파이긴합니다!!!


@Component
@RequiredArgsConstructor
public class AsyncDeleteProcessor {
@Transactional(propagation = Propagation.REQUIRES_NEW)
public class TransactionalDeleteProcessor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 느낌일까요?

    public void invoke(final Long memberId, final DeleteInvoker invoker) {
        tripDeleteInvoker.delete(memberId, tripRepository);
    }
  • 함수 호출시
transactionalDeleteProcessor
     .invoke(
           event.getMemberId(),
          (Long memberId, TripRepository repository) -> repository.deleteByMemberId(memberId)
);

@jjongwa jjongwa closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Member or Trip 삭제에 따른 트랜잭션 분리 메서드 비동기 처리
4 participants