[HANSEL-119] Set Version Column & Refactor RegisterReviewStats

[Shanate]

📄 [HANSEL-119] Set Version Column & Refactor RegisterReviewStats

Jira : HANSEL-119

✨ 변경 사항

• 기능 추가/변경 설명

• 리뷰를 동시에 진행할 때 평점 그리고 갯수에 대해 동시성 이슈가 생길 수 있습니다. 따라서 해당 문제를 해결하기 위한 코드입니다.

• 리뷰 작성, 수정, 삭제보다는 조회가 더 자주 일어날 서비스로 예상하고 있기 때문에 해결 방법으로는 Optimistic Lock을 사용합니다.

• Review에 의한 동시성 발생이긴 하지만, 중요시 여겨야 하는 부분은 동시성 문제가 어디서 발생하는지입니다. 리뷰 평점과 갯수는 Workspace에 영향을 미치기 때문에, Version을 ReviewEntity가 아닌 WorkspaceEntity에 추가해 버전을 관리하게끔 했습니다.

• [25.01.05 코멘트 추가] WorkspaceAdapter에서 리트라이 로직을 구현했습니다. 해당 로직 플로우는 다음과 같습니다. 버전 충돌이 발생했을 때 ObjectOptimisticLockingFailureException로 예외를 터뜨립니다. 그리고 현재 스레드를 대기시킨(10ms) 후 다시 로직을 수행하며, 정상적으로 수행될 때까지 반복합니다. 대기시간을 설정한 이유는, 곧바로 재시도를 하면 또 충돌할 가능성이 많기 때문입니다.그리고 해당 리트라이 로직을 반복하는 도중에 외부 인터럽트가 발생한다면 Internal Server Maintenance라는 에러 메시지를 발생하도록 설정했습니다. 이 익셉션은 유저에 의해 발생하는 것이 아닌, 내부 개발 작업 중에 발생할 수 있는 상황이기 때문입니다. 어떤 메시지를 출력하는 것이 좋을지 생각해보고 찾아본 결과, DB 상태를 변경하는 것과 같은 개발자가 어떠한 인터럽트를 가하거나 수정을 했을 경우 발생할 수 있다고 하였기에 다음과 같은 메시지를 출력하는 것이 나은 판단이라고 생각했습니다.

📅 작업 일정

• Expected MD: 0.2 MD
• Actual MD: 6 MD

Difference reason (If correct, no need.)

• None.

✔️ 확인 사항

• 코드가 잘 작동하는지 확인했나요?
• 새로운 기능에 대한 테스트가 추가되었나요?
• 문서가 업데이트되었나요?

낙관적 락을 사용해야 하는 당위성

https://equatorial-break-b4c.notion.site/17ab3c2f1d1980268e3dfabc76284994?pvs=73
(해당 리팩토링이 끝날 때까지 계속 업데이트)

[weejinyoung]

Optimistic Lock 을 사용하면 재시도 관련 로직을 따로 만들어줘야하는 등 공수가 많이 드는 것으로 알고 있어요
동시성 제어의 수단으로 Optimistic Lock 을 선택하신 이유가 무엇인지 궁금해요
처리성보다 조회성 io 가 많다는 이유 이외에!

[weejinyoung]

domain 모듈은 모든 모듈이 참조하고 있는 모듈이기 때문에 implemantation 을 할 때 더욱 명확한 이유가 있어야 할 것입니다
이 종속이 있는 이유가 무엇인가요?

[Shanate]

OptimisticLockException을 사용해보고자 했습니다.
Domain에 주입한 까닭은, Domain test에서 ReviewCountingTest를 진행할 것이기 때문에 해당 'jakarta.persistence:jakarta.persistence-api:3.1.0'을 implementation해야 하지 않을까 하는 생각에 종속을 했습니다

[weejinyoung]

우리가 처음 정했듯이 domain 모듈은 외부 기술의 종속을 최소화하고 중요한 비즈니스 로직을 담은 순수 자바 개념 객체를 관리하는 책임을 가지고 있어요
다만 말씀하신 ReviewCountingTest 는 데이터베이스 기술이 필요한 것이라... persistence 모듈의 책임이 아닐까 싶습니다

[Shanate]

그렇다면 궁금한 것이 있는데요.
기존 ReviewCountingTest라는 클래스를 만들어서 리뷰 로직 그러니까 CRUD가 정상 작동하는지를 테스트 했었는데요.
그렇다면 동시성 이슈는 해당 도메인에서의 비즈니스 로직이 아닌 DB와 연관된 이슈라고 봐야하는 것이기 때문에 Persistence에서 테스트를 진행해야 하는 것일까요?

[weejinyoung]

맞아요 리뷰의 동시성 문제 해결은 비즈니스라고 보기 어렵죠!
동시성 문제는 데이터베이스 상에서의 문제라고 생각합니다

[weejinyoung]

version 은 데이터베이스 단에서만 사용되는 것으로 알고있습니다
이 version 이 도메인인 클래스의 필드에 있어야하는 이유가 있나요?

[Shanate]

또 도메인과 엔티티를 하나의 관계라고 생각한 것 같습니다.
분리해서 생각해야 하는데 이를 인지하지 못하고 있었습니다.

[Shanate]

Optimistic Lock 을 사용하면 재시도 관련 로직을 따로 만들어줘야하는 등 공수가 많이 드는 것으로 알고 있어요 동시성 제어의 수단으로 Optimistic Lock 을 선택하신 이유가 무엇인지 궁금해요 처리성보다 조회성 io 가 많다는 이유 이외에!

기존에 동시성 문제에 대해서는 어렴풋이 Lock을 통해서 해결한다고 들었습니다. 그래서 이번에 중점적으로 고민을 했던 것이 Optimistic과 Pessimistic인 것인데, 조사한 바로는, 동시성 문제의 빈도입니다.
두 번째로는 트랜잭션 처리 시간입니다. Optimistic은 애플리케이션 단에서 처리를 하며 실질적으로 Lock이 없기 때문에 성능 영향을 덜 받으며, Pessimistic은 충돌이 거의 없는 대신에 Lock으로 인해 트랜잭션 처리 시간이 비교적 느린 것으로 알고 있습니다.

사실 근본적인 이유라고 한다면, 조회가 처리보다 많다는 것을 이유로 들었는데, Optimistic이 적합하지 않다면 어떤 이유인지에 대해서 약간의 힌트를 주시면 감사하겠습니다.
추가적으로, Retry 로직을 생각해보지 않았는데 이번 리팩토링을 위해 여러 블로그를 찾아봤을 때, 데이터처리 관점에서는 문제 없겠지만 500 에러를 터뜨리며 프로그램의 신뢰성을 낮출 것이라고 보아, Retry로직은 필수적인 것을 알게 됐습니다.
그래서 진영님이 말씀하신 공수가 많이 드는 것을 Optimistic Lock을 사용하기로 결정한 이후에 깨닫게 됐습니다.

[harvartz]

잘 봤습니다.. 낙관적 락을 적용했는데 잘 해결이 되었나요..? 낙관적락은 진영님이 말씀하신 것처럼 재사용 로직을 구현해야 한다고 들었었어요! 그부분은 아직 코드에 나와있지 않는 것 같네요
그리고 이런 조회가 자주 이루어지는 부분은 비관적 락보다는 낙관적 락을 많이 사용한다고 들었습니다. 비관적락은 확실히 동시성 이슈가 나지 않으나, 제가 경험한 바로는 느리더라구요.. 이 부분에 대해서도 진영님 생각이 궁금하기도 합니다 @weejinyoung

그리고 위에 문제에 대해서 동시성 이슈가 어떻게 생기는 지 더 자세한 문서가 있으면 좋을 것 같습니다. 저도 공부해보고 싶은 욕심이 있어서 정리하신 노션 링크를 올려주시면 참고해보겠습니다!

[weejinyoung]

단순히 Retry 로직을 구현하는 것 이외에도 이후에 해당 로직을 관리하는 공수도 생각해야해요
혹시 조회할 때에도 락을 걸어야하나요?

[Shanate]

잘 봤습니다.. 낙관적 락을 적용했는데 잘 해결이 되었나요..? 낙관적락은 진영님이 말씀하신 것처럼 재사용 로직을 구현해야 한다고 들었었어요! 그부분은 아직 코드에 나와있지 않는 것 같네요 그리고 이런 조회가 자주 이루어지는 부분은 비관적 락보다는 낙관적 락을 많이 사용한다고 들었습니다. 비관적락은 확실히 동시성 이슈가 나지 않으나, 제가 경험한 바로는 느리더라구요.. 이 부분에 대해서도 진영님 생각이 궁금하기도 합니다 @weejinyoung

그리고 위에 문제에 대해서 동시성 이슈가 어떻게 생기는 지 더 자세한 문서가 있으면 좋을 것 같습니다. 저도 공부해보고 싶은 욕심이 있어서 정리하신 노션 링크를 올려주시면 참고해보겠습니다!

9일까지 1차적으로 완성을 한다고 했는데, 이게 안일하게 생각했던 것 같습니다.
비교적 구현이 쉽다고 했는데, 이게 들어갈수록 고려해야 하는 것이 한두가지 계속 생겨서 죄송하지만 1차적으로 코드를 완성하지 못했습니다. 처음에는 serviceImpl 단에만 걸어두면 되겠지라는 생각을 했는데, 이게 그렇게 되면 500을 고려하지 못했던것이되더라고여

[Shanate]

단순히 Retry 로직을 구현하는 것 이외에도 이후에 해당 로직을 관리하는 공수도 생각해야해요 혹시 조회할 때에도 락을 걸어야하나요?

아... Retry 로직을 구현하는 것이 아닌 그걸 또 처리를 해야하는거군요. 혹시 제가 이해한게 맞는지 답변 부탁드립니다.
version이 맞지 않았다 -> 누군가 해당 트랜잭션을 사용하고 있어 동시성에 문제가 생겼다. -> 이를 Retry 로직을 사용해 최신 상태의 DB를 계속 조회하며 동시성 문제를 해결한다. -> Retry 횟수가 넘어갈 경우 Exception 처리를 하지 않을경우 500에러가 발생한다. -> 만일 Retry 횟수가 넘어갔을 경우 사용자에게 오류 메시지를 반환한다.