-
Notifications
You must be signed in to change notification settings - Fork 16
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
이벤트에 Transactional Outbox Pattern 적용 #757
Conversation
📝 Jacoco Test Coverage
|
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.
멋지네요 라온 & 홍고
추가적으로 몇가지 궁금한게 있어 질문 남깁니다.
우선 해당 작업이 왜 필요했는지, 기존의 어느 부분에서 크리티컬한 문제점이 있어 개선을 하게 되었는지 설명이 조금 부족한것 같아요.!(연결된 이슈에도 설명은 딱히 없더라구요..)
그리고 이벤트 발행 로직과 이벤트를 같은 트랜잭션에 묶어서 원자성을 지키기 위해
이 부분도 잘 이해가 가지 않습니다.! 이벤트 발행과 해당 이벤트를 같은 트랜잭션에 묶는다는 건가요? 원자성
이란 키워드를 어떤 의미에서 썼는지는 대충 이해가 가지만 그게 정말 원자성이 맞는지는 잘 모르겠습니다.! 이 부분도 조금 더 설명해 주시면 좋을 것 같아요..!
현재 저희의 아키텍처 구조에서 이벤트 큐를 사용한 아웃박스 패턴이 최선의 방법이었는지, 다른 선택지는 어떤게 있었고, 각각 어떤 단점이 있어 반려하게 되었는지 궁금합니다!
지금 저희는 모듈 하나에 모든 로직이 구현되어 있는데요, 그 말은 이벤트 발행과 처리를 모두 같은 서버에서 처리하고 있다는 의미입니다.
이러한 상황에서 굳이 아웃박스라는 테이블을 만들고, 이벤트 큐를 만들어서 주기적으로 쿼리를 실행해 이벤트를 가져와 해당 로직을 실행해야 할 이유가 있을까요?
트랜잭션 아웃박스 패턴은 이벤트의 발행과 실행 로직이 서로 다른 서버에 있고, 해당 서버 간의 통신 중 이벤트가 유실될 경우를 대비한 방안이라고 이해하고 있습니다.(분산 아키텍처 -> 더 나아가면 MSA 환경이겠죠..!)
때문에 같은 서버에서 처리하는 현 상황에서는 큰 효과가 없이 그저
- delete 로직에서 일괄적으로 DB 쿼리가 2번씩 추가된다.
- 이벤트에 대한 처리가 (최대)2초 뒤에 실행된다
는 단점밖에 생기지 않을 것 같아요.
그리고 굳이 이렇게 DB와 연동하지 않고 인메모리에 큐를 만들어 스케줄러로 처리해도 될 것 같구요.!
아니면 접근속도가 더 빠른 레디스에 저장하는 것도 방법이 될 수 있을 것 같습니다.
이렇게 다양한 방안이 있을텐데 굳이 DB에 데이터를 추가하고 삭제하는 방식으로 구현한 이유가 궁금합니다.!
그리고 저희는 다중 서버 구조인데 이렇게 아웃박스 테이블을 만들고 이벤트 큐에서 주기적으로 findAll을 해오게 되면 각각의 서버에서 동일한 이벤트 처리를 하게 되는 상황도 오지 않을까 싶은데 이 부분은 어떻게 생각하시나요?(제가 코드를 완벽하게 이해하지 못했을 수도 있습니다..!)
이벤트 큐가 물론 현업에서 많이 쓰이고 학습해보고 싶은 로직임은 이해합니다.
하지만 지금 저희 서비스의 입장에서 봤을땐 해당 로직의 추가가 오히려 성능상 마이너스 요소가 된다고 생각하는 입장이기에 dev로의 머지는 반대합니다.!
조금만 더 의견 나눠보고 디벨롭해서 반영하는 쪽으로 진행해 보면 좋겠습니당
|
||
@AfterReturning("@annotation(org.springframework.transaction.event.TransactionalEventListener)") | ||
public void check(final JoinPoint joinPoint) { | ||
System.out.println("CompletedEventChecker.check"); |
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.
chk
final Outbox outbox = queue.poll(); | ||
final OutboxToEventMapper outboxToEventMapper = mappers.stream().filter(mapper -> mapper.is(outbox.getEventType())) | ||
.findFirst() | ||
.orElseThrow(() -> new IllegalArgumentException("이벤트 타입에 해당하는 매퍼가 없어요오")); |
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.
이대로 놔둘건가요?
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.
레전드
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.
exception code 하나 만드시죠.!
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class EventQueue { |
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 | ||
@Entity | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
@SQLDelete(sql = "UPDATE outbox SET status = 'DELETED' WHERE id = ?") |
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.
아웃박스 테이블도 soft delete를 하는 이유가 있을까요? 본래 아웃박스의 의도와는 조금 달라질 수도 있다는 생각이 들어서요.!
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.
status가 필요해서 재활용했는데, baseEntity의 status대신 아웃박스의 Status를 별도로 만드는 게 좋을까요?!
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.
아 그럼 그대로 가도 될 듯 합니다
publisher.publishEvent(new TripDeleteEvent(tripId)); | ||
|
||
final String payload = PayloadToEventMapper.toJson(new TripDeleteEvent(tripId)); | ||
outboxRepository.save(new Outbox(EventType.TRIP_DELETE, payload)); |
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.
이렇게 되면 결국 삭제 로직에서 DB 접근이 추가적으로 필요한 상황인데 트랜잭션 보장을 위해서 해당 방법이 가장 합리적일까요?
이 외에 다른 방식을 고민해 본게 있을까요?
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.
일단 외부 db에 저장한다는 것은 필수 조건으로 깔고 갔습니다! 아래 댓글 참고오오오
@Scheduled(fixedRate = 2000) | ||
public void offerSavedEvent() { | ||
queue.addAll(outboxRepository.findAll()); | ||
} |
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.
-
findAll의 호출 기준을 2초로 둔 이유가 궁금합니다!
-
지금 저희는 다중 서버인데 그럼 각 서버마다 큐를 운영하는건가요?
그러면 큐에서 빼면서 호출하는 로직도 각각 서버마다 진행되는건가요?
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.
아래 댓글에 한 번에 써둠!!!
boolean is(EventType type); | ||
T toEvent(Outbox outbox) throws JsonProcessingException; |
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.
여기 final 못붙이나요
@jjongwa 원자성을 지킨다는 것이 무슨 의미인가?
이벤트 발행과 해당 이벤트를 같은 트랜잭션에 묶는다는 건가요? <- 맞습니다!!
deleteTrip()이 제대로 수행되었다면, Trip도 삭제되고, 이벤트도 제대로 발행되어야한다는 의미에서 원자성이라는 단어를 언급했습니다. 아웃박스 테이블을 만든 이유물론 2번의 경우는 Trip을 삭제하는 코드 다음에 이벤트를 발행하기에 발생하지 않긴 합니다.ㅎㅎ
delete 로직에서 일괄적으로 DB 쿼리가 2번씩 추가된다.
이벤트에 대한 처리가 (최대)2초 뒤에 실행된다
각각의 서버에서 동일한 이벤트를 처리하게 되는 것 아닌가?
맞습니다!!! 일단, 이벤트가 delete만 있기에 이벤트가 두 번 처리될 때 치명적인 오류가 발생하지는 않아 이대로 구현했습니다! 그렇다면 이벤트 테이블에 파티셔닝을 적용하고, 해시값을 기준으로 적합한 테이블에 이벤트를 저장하는 방식은 어떨까요? 각 서버가 조회해올 테이블을 지정해준다면, 이벤트가 두 번씩 처리되는 것을 막을 수 있을 것 같습니다! 너무 오바인가요?! 하하하!!! 정말 트랜잭셔널 아웃박스 패턴이 최선이었는가? 다른 방식은 없었나?음... 일단 delete이벤트만 있는 현재 상태에서는 이벤트 유실을 막기위해 트랜잭셔널 아웃박스 패턴 대신, 주기적으로 스케줄링을 돌리며 delete가 되지 않은 엔티티를 찾아 지우는 방식이 떠올랐었는데요! 예시만 봐도... 너무 복잡하죠! Trip과 연관된 객체가 너무 많을 뿐더러 인덱스가 설정되지 않은 Status를 기준으로 조회를 한다면 쿼리 비용이 많이 들거라고 생각했습니다. 전 이 방법과 트랜잭셔널 아웃박스 패턴외에 이벤트 유실을 막기 위한 방법이 떠오르지 않았긴합니다ㅠ.ㅠ 만약 현재 방법보다 더 좋은 방법이 있다면 바꿀 의향 10000% 입니다! |
디노가 맛깔나게 반박해줄것같아 설레네요1! 기대하겠습니다!!!!!!!!!!!!1 |
빨리 답글을 적었어야 하는데 제가 너무 늦게 왔죠..죄송합니다ㅡㅜ(예비군도 했고 저희 수료식도 있었으니까 봐주시죠 하하) 반박?추가설명? 차근차근 들어가 보겠습니다..
이렇게 두 가지 경우를 예를 들어 설명해 주셨는데요.! Transactional Outbox Pattern (이하 top라 하겠습니다..) 도입에는 자 그럼 이제 먼저,
제가 앞선 리뷰에서 기존의 로직 -> tripDelete 완료 & 이벤트 발행 (0.3초) -> 이벤트 처리 (1.7초) 변경한 로직 -> tripDelete 완료 & 이벤트 발행 (0.3초) -> 2초마다 스케줄링 로직 실행(최대 2초) -> 이벤트 처리 (1.7초) 뭔가 기능 추가를 위해 문제가 될 만한 상황을 더 크게 만들고 이를 해결하려는 것처럼 보여서 조금 찝찝합니다.. 저는 오히려 홍고가 마지막에 적어 놓은 결론적으로 요약하자면
이 제 의견입니다. 그래서 저는 hard delete의 기간을 고려해 pollEvent 로직의 실행 주기를 재조정 하는 작업을 거친다면 조금 더 이번 PR에 의미가 생길 것 같아요! 물론 그냥 저의 의견입니다!!! 이해가 안되거나 궁금한게 있는데 빠르게 답을 주고받고 싶다면 dm으로 연락 주세요..! |
아아 그렇다고 뭐 |
저도 리뷰에 대한 답이 많이 늦었네요ㅠ 위에 디노가 말씀주셨던 데이터 분산 처리 환경이라는 전제. 라는 점은 모두 이해했습니다. 반박?할 말은 떠오르지 않네요 ^_^
그렇다면, 그래서구현 방법을 단순하게 보자면 아래와 같아요
top 패턴에 대한 반박?을 하셨는데, 새로 올라온 PR에서는 아웃박스 패턴은 그대로 적용되어 있어서 어느 정도 동의한다고 봐도 될까요 .. ? 저도 큐를 통한 스케줄링 처리는 부가적인 선택이라고 생각해요. 스케줄링 주기를 얼마로 가져갈까도 어떤 로직이냐에 따라 달라질 수 있겠죠! 큐 도입. 디노가 올려주신 PR에서는 TripSerivce에서 삭제되지 않는 이벤트를 처리하고 있어요, 물론 패키지 의존성 신경쓰지 말라고 하셨지만, Trip 이외 Member, Image에 대한 스케줄링 로직도 필요한데, 이 역할을 담당하는 클래스 또한 필요하다고 생각하는데 여기서 그냥 큐로 가져와서 앞에서부터 하나씩 처리한다! 라는 생각에서 나왔어요!! 스케줄링 주기 2초. 추가적으로, 클래스가 많아진 이유에 대해서 변명을 해보자면, 이는 Outbox를 추상화하면서 발생한 문제라고 생각해요. 올려주신 디노의 PR과 달아주신 커멘트들을 읽어보며 답글을 남겨봤는데 .. |
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.
고생하셨습니다!!! 홍고 라온!
@jjongwa 1. top 적용을 통해 해결할 수 있는 상황 전제가 너무 극단적임이건 제가 예시를 잘못들었던 것 같네요! 디노말대로 서버 가용성에 즉결되는 로드밸런싱, DB Replication과 비동기 로직은 다른 비교 선상에 두는 게 맞다고 생각합니다!!! 2. 스케줄링 주기
여긴 소통의 오류가 있었던 것 같네요! 디노가 현재 PR에 대한 단점으로 그래서 이벤트 로직은 빨리 처리되지 않고, 근시일내로 수행되면 될 것 같다고 말씀드린겁니다! (근시일 이라는 표현도 수정하겠습니다... 저도 db에 부하가 가능하지 않는 선이라면 언제든 이벤트를 수행해도 괜찮다는 입장입니다...) 2초로 둔 이유는, hard delete가 수행되기 이전 & 서버에 부하가 걸리지 않는 로직 반복 횟수를 만족한다면 몇 초든 괜찮다고 생각했기에 임의로 둔 값입니다. 기간을 크게 잡지 않은 이유는 행록이 hard delete를 수행하는 시간을 정하지 않았기 때문에 혹시라도 이후에 hard delete가 수행될 때 그 기간을 넘기는 것을 우려했습니다. 3. top 꼭 분산환경에서만 써야 하는가ㅎㅎ 챗지피티 센세 첨부 감사합니다 이벤트 처리의 방향은 ... ?!
하핫... 라온이 image삭제를 언급한 거 저 때문인 것 같네요! s3에 이미지 리스트를 올리다가 예외가 나면 이미지 삭제 이벤트를 추가하는 것을 고려하고 있었습니다! 디노와 라온에게 한 번 언급했었는데 라온이 이걸 염두하고 하신 말 같네요 여러개의 OutBox를 두어도 상관 없다고 생각합니다. 이거 event마다 각자의 outbox테이블을 만들자는 의미가 아니라 pr처럼 type만 구분해서 하나의 outbox테이블에 저장하자는 의미맞죠? 굳이 이벤트를 저장해야 할까? 이것도 그럼 outbox 테이블 자체를 없애자는 말이 아니라, 이벤트의 정보 전부를 저장하지 말고, 타겟도메인아이디와 이벤트type만 저장하자는 말 맞죠?!!!! 디노 PR 잘 보고 왔습니다!
이게 디노의 최종의견인거 맞나요? 전 좋습니다!!! + 혹시 더 논의가 필요하신 것 같으면 아예 날을 잡아서 다같이 게더로 만나는 게 어떨까요?!!!!!!!!1 |
소통이 좀 느리지만, 패키지 의존성 뒤로 진지한 토론을 하는거 같아 재밌네영 스케줄링 주기이거는 저희 셋 모두 특정 기준을 통해 주기를 정하자 까지 결론이 도달한거 같네요!!! OutBox의 추상화디노가 말씀해주신 부분이 모두 추상화와 관련된 부분인거 같아서 섹션을 좀 나눠서 의견을 보충해봤습니다 ~ 1. 이미지 삭제
이미지 관련해서는 홍고가 말씀해주신게 맞아요! 현재 있는 로직은 아니지만, 2. 큐
전 코멘트에서 말한 것처럼 큐에 가져와서 처리하는 방법이 아닌 그냥 리스트로 스케줄링을 통해 수행하는거는 동의! 인데
3. 페이로드에 대한 직렬화
테이블 필드에 어디까지 저장할 수 있을까! 이 부분은 한번도 고민해보지 않았네요.. 물론 디노가 올려주신 PR처럼 Long 타입으로 제한해두면 편하겠지만 .. 앞서 말씀드렸던 것처럼 이벤트에 다양한 필드가 들어올 수 있을거라고 생각해서 직렬화 로직을 넣었습니다! 위에서 언급한 이미지 삭제 이벤트도 List으로 imageId 리스트를 필드로 가지면 좋을거 같다고 생각했어요. (물론 imageId를 하나씩 저장하는 방법도 있긴함) 그치만 무조건적으로 필요한 로직은 아닌 것에 동의, 하지만 추상화해놓으면 좋지 않을까요 ?!
이건 부하테스트 수행 시, 사용자가 가질 수 있는 최대 여행 수를 모르니 적절한 기준이 없어 테스트하는데 어려움을 겪어서 정한 수치였습니다! 4. 분기문을 통한 로직 처리
첫번째 문장은 이해가 잘 안가요 히히히 .. 이 부분도 홍고가 디노 말처럼 스케줄링 로직에서 분기 처리하는 방식이 저도 당연히 간단하다고 생각합니다! 근데 이벤트가 늘어날수록 분기 처리 로직이 늘어날 것이라고 생각해서 추상화를 한거였어요! 저희 추천 알고리즘도 사실 지금 당장 좋아요 순으로 정렬하는 알고리즘 밖에 없음에도 불구하고 추상화가 되어있잖아요, 그냥 그런 의도였다고 생각해주시면 좋을거 같아요. 그래서 저는 if문을 통한 분기처리보단, 타입에 따라 올바른 이벤트 로직이 처리되도록 수행되면 좋을거 같아요 🙃 (현재 PR에서는 어쨌든 이 코멘트의 결론은 물론 간단간단하게 구현할 수 있다! 하지만 난 추상화하고 싶다 ~
그리고 저도 이관 관련해서 게더에서 함 얘기하면서 이것도 같이 이야기해보면 좋을거 같아요 ~~! ~! |
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.
답장이 늦었네요.. 몸이 좋지 않아 좀 앓아 누웠습니다..
제 의견에 대해 두 분 다 충분히 이해한 것 같구 제가 더 이상 추가로 주장할 내용도 없어서 나머지는 두 분의 선택에 맡기겠습니다.!
맨날 대면으로 토론하다가 이렇게 PR에서 의견 남기니까 나름 재밌었습니다ㅎㅎ
이만 approve 할께요~~!!
(물론 게더에서 추가 논의하는 것도 좋아요~ 적극적 참여 가능)
그ㅡㅡㅡ래도 마지막으로 말하자면
지금 상황에서 추상화는 오버엔지니어링이다 는 입장......................!하하하
📄 Summary
🙋🏻 More
기존에 이벤트 호출 로직(TripService.deleteTrip())에서 이벤트를 바로 발행하고 있었습니다.
이벤트 발행 로직과 이벤트를 같은 트랜잭션에 묶어서 원자성을 지키기 위해 트랜잭셔널 아웃 박스 패턴을 적용해봤습니다!
(트랜잭셔널 아웃 박스 패턴에 대한 설명은 링크를 참고해주세요)