[JDBC 라이브러리 구현하기 - 4단계] 연어(황재현) 미션 제출합니다. #577

nuyh99 · 2023-10-09T07:57:56Z

도기 안녕하세요! 항상 좋은 피드백 감사합니다.
급하게 구현해서 제출합니다...!
4단계도 잘 부탁드려요😁

구현 목록

Transaction synchronization 적용하기
- DataSourceUtils를 사용해서 커넥션을 스레드 로컬에 저장해서 사용하도록 했습니다.
트랜잭션 서비스 추상화하기
- UserService의 두 가지 구현체(비즈니스 로직, 트랜잭션 로직)를 만들었습니다.
- 여전히 각 서비스마다 Tx~Service를 구현해야 하지만 비즈니스 로직과 트랜잭션 로직이 분리됐습니다.

kdkdhoho

안녕하세요 연어~ 드디어 마지막 4단계네요!
건강상 이슈로 빠르게 리뷰남기지 못해 죄송합니다.. 이번 미션 내내 빠르게 리뷰 남기고 싶었는데 그러지 못한 것 같네요 😢

이번에도 몇 가지 이야기나누고 싶은 것들이 있어 RC 남깁니다.
확인 부탁드려요~

jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java

kdkdhoho · 2023-10-10T03:17:01Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+
+    @Nullable
+    public static Connection getResource(final DataSource key) {
+        final Map<DataSource, Connection> connections = getConnections();


connections보다 resource라는 이름은 어떠신가요?
ThreadLocal 변수명도 resources 이네요!

현재는 기본으로 주어진 resources의 타입이 Map<DataSource, Connection>이라서 Connection에 대한 컬렉션이라고 생각합니다.

하지만 실제 TransactionSynchronizationManager는 Connection 뿐 아니라 Object를 받을 수 있게 돼있습니다!
따라서 지금의 resources도 어떤 자원이든 더 받을 수 있도록 타입이 Map<DataSource, Object>나 Map<Object, Object> 등으로 바뀔 수 있다고 생각했어요. 그래서 resources라는 네이밍이 확장성 있다고 생각했습니다.

하지만 아래의 로직들은 getConnections()라는 메서드로 커넥션들을 리턴 받아서 쓰고 있는데요.
Connection의 컬렉션을 리턴하기 때문에 connections라는 네이밍이 맞다고 생각했어요!
그리고 Map<DataSource, Object> 등으로 바뀌어도 getConnections()만 바꿔주면 커넥션들의 집합을 리턴한다는 시그니처는 변하지 않아도 되기 때문에 다른 코드들은 변경에서 안전하다고 생각했습니다.

말씀해주신 resource는 단수라서 개인적으로 Connection의 집합과는 부합하지 않는다고 느껴졌습니다!

정리하면,
현재 스레드만의 저장 공간: resources
현재 스레드만의 저장 공간 속의 커넥션 집합: connections
위와 같이 생각해서 네이밍했습니다!

현재는 기본으로 주어진 resources의 타입이 Map<DataSource, Connection>이라서 Connection에 대한 컬렉션이라고 생각합니다.

정확히 말하면 resources의 타입은 ThreadLocal<T>이고, 그 안에 제네릭 타입으로 Map<DataSource, Connection>이 될 것 같아요.
따라서 제가 판단했을 땐 resource는 T가 될 것이라고 생각했기 때문에 제안드렸습니다.

말씀해주신 resource는 단수라서 개인적으로 Connection의 집합과는 부합하지 않는다고 느껴졌습니다!

제가 알기로 ThreadLocal에 하나의 쓰레드당 하나의 Connection을 각각 저장하는 식으로 알고 있기에, 어떻게 보면 단수가 좀 더 적잘하지 않나 생각이 듭니다!

논외로, 어차피 DataSource는 싱글톤 개념이기에 단순히 ThreadLocal<Connection> 이어도 되지 않을까? 하는 생각이 들어 실제 변경하고 테스트도 돌려보았는데 모두 통과를 하더라구요. 왜 기본적으로 제공되는 코드에선 Map<DataSource, Connection>으로 주어졌을까요? 🤔

우선 하나의 스레드에 여러 개의 Connection을 저장할 수도 있습니다!
물론 현재는 한 DataSource에 대해 하나의 Connection만 저장할 수 있지만, 여러 DataSource의 구현체를 사용해서 여러 커넥션을 하나의 스레드에서 사용할 수 있습니다.

DataSource의 구현체는 싱글톤이지만 DataSource는 단순 인터페이스라서 말씀하신 부분은 잘 모르겠네요ㅠ

논외로 말씀하신 부분은,
원래의 TransactionSynchronizationManager는 Map<Object, Object>로 저장해둡니다.

따라서 RDBMS에 대한 커넥션 뿐 아니라 메세지 큐나 다른 서브 시스템과의 커넥션도 저장할 수 있습니다.
그래서 스프링 측에서 여러 커넥션들을 하나의 트랜잭션으로 관리할 수 있어요!

지금 현재의 트랜잭션 매니저에서는 예를 들어 두 개의 MySQL을 사용할 때 각각의 커넥션을 하나의 트랜잭션으로 처리할 수 있겠네요!

결론은 다시 돌아와서 현재 여러 개의 Connection만 저장할 수 있는 구조이기 때문에 저렇게 네이밍 했어요!

논의로 말씀드렸던 부분은, 단일 스프링 애플리케이션에서 2개 이상의 DataSource를 사용할 수 있다는 점을 뒤늦게 알게 되었네요 😅

실제 TransactionSynchronizationManager에 가깝게 구현하려는 연어의 의도를 제가 몰랐던 것 같아요. 👍

kdkdhoho · 2023-10-10T03:23:12Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+    @Nullable
+    public static Connection getResource(final DataSource key) {
+        final Map<DataSource, Connection> connections = getConnections();
+        return connections.get(key);
+    }
+
+    private static Map<DataSource, Connection> getConnections() {
+        Map<DataSource, Connection> connections = resources.get();
+
+        if (isNull(connections)) {
+            connections = new HashMap<>();
+            resources.set(connections);
+        }

-    public static Connection getResource(DataSource key) {
-        return null;
+        return connections;
    }


이렇게 되면 ThreadLocal에 null, null로 자원이 할당되고, 결국 getResource에서 나가는건 null이 될텐데 이렇게 해주신 이유가 있나요??

우선 getResource()는 @Nullable API입니다.
getResource(key)를 호출했을 때 해당하는 자원이 없으면 예외 상황이 아니라 null을 리턴하는 것을 의도했구요!

null, null이 할당된다는 것은 어떤 부분인 지 모르겠습니다!
if(isNull(connections)) { 구문에 걸렸을 때는 스레드 로컬이 비어있기 때문에 새 HashMap만 생성되게 되고 아무것도 put 되지 않습니다.

null, null로 자원이 할당되고

Map<DataSource, Connection> connections이 null이라면 new HashMap<>()만 할당해주게 되고, 결국 중요 자원인 DataSource와 Connection는 null로 할당된다는 말이었어요!

이러면 사실 resources.get()의 결과가 null인거랑 다를 게 없어보여서 남긴 리뷰였습니다!

'똑같은데 뭘해도 상관없지 않나요?' 라는 생각이 들 수 있습니다. 하지만 빈 HashMap이 할당되어 있어서 실제로 ThreadLocal에 자원이 정상적으로 할당되어있는지 확인할 때 놓칠 수 있는 여지를 준다고 생각하기에, 빈 HashMap을 할당하는 게 괜찮나 의문이 들었어요!

중요 자원인 DataSource와 Connection은 null, null 상태가 아니라 HashMap 내부의 Map.Entry 인스턴스는 전혀 존재하지 않는 상태라고 생각됩니다!
그리고 사용자가 getResource(key) 메서드를 호출할 때 new HashMap<>()이 ThreadLocal에 set 돼있지 않다면 NPE가 발생하게 됩니다.

resources.get()의 결과가 null인 것까지는 괜찮아요!
근데 필요한 메서드는 Connection getResource(DataSource key)이고, 만약 ThreadLocal에 set된 것이 없다면 맵에서 파라미터로 받은 key에 대한 값을 꺼내려고 할 때 NPE가 발생합니다.

없는 자원을 조회할 때 NPE 등의 예외를 발생시킬 지 null을 리턴할 지에 대한 문제인데 저는 null을 리턴하는 쪽을 택했습니다!

예외는 비용이 드는 행위이기 때문에 안 터뜨릴 수 있으면 최대한 안 터뜨리려고 합니다. 차라리 Optional API를 쓸 것 같아요!

실제 트랜잭션 매니저에서는 바인딩할 때만 null이면 new HashSet<>()을 할당해줍니다. 저는 getConnections() 메서드를 트랜잭션 매니저의 인터페이스에서 공통적으로 쓰기 위해 null이면 항상 new HashSet<>()을 할당했습니다.

실제로 ThreadLocal에 자원이 정상적으로 할당되어있는지 확인할 때에 대한 기능은 현재 없습니다. 지금은 getResource(key)를 통해서 해당 DataSource의 커넥션이 있는지만 확인이 가능합니다. 만약 저 행동이 필요하다면 hasResources() 메서드를 만들고 Map 객체가 null이거나 empty인지를 판단하도록 구현할 것 같아요. 이러면 문제가 없지 않나요??!

실제 TransactionSynchronizationManager에 가깝게 구현하려는 연어의 의도를 몰랐습니다..! 👍

kdkdhoho · 2023-10-10T03:25:07Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+        if (connections.containsKey(key)) {
+            throw new IllegalStateException("이미 커넥션이 존재합니다.");
+        }


방어 로직 좋네요 👍
근데 문득 드는 생각인데, ThreadLocal에 새로운 자원을 할당하는 경우는 없을까요?
그럴 땐 새로운 메서드로 처리해주면 될까요?

새로운 자원을 할당하려고 할 때 단순히 덮어씌워줄까?하고 생각해봤었습니다.
근데 ThreadLocal에 저장하는 자원들은 정리 해줘야하는 자원(Connection)이기 때문에 예외를 발생시켜서 기존 자원을 종료시키길 바랬습니다.

ThreadLocal에 새 자원을 할당한다는 것은 한 DataSource에 대해 두 개 이상의 Connection을 연결한다는 말씀이시겠죠??
지금 만든 트랜잭션 기능에서 requires new 등을 제공한다면 한 스레드에서 두 개 이상의 커넥션이 필요할 것입니다.
그때는 resources의 타입이 Map<DataSource, List<Connection>> 등과 비슷하게 될 것 같아요!

하지만 지금은 단일 트랜잭션만 지원하기 때문에 한 DataSource에 대해 여러 커넥션이 필요가 없다고 생각해요...!

ThreadLocal에 새 자원을 할당한다는 것은 한 DataSource에 대해 두 개 이상의 Connection을 연결한다는 말씀이시겠죠??

원래 의도는 덮어씌어주는 경우를 의미했습니다!
하지만 연어 말씀대로 현재는 단일 트랜잭션만 지원하기에 지금처럼 방어로직을 가져가는게 좋아보이네요 👍

현재도 여러 커넥션을 사용할 수는 있지만 동의합니다!
실제 트랜잭션 매니저에서는 예외를 발생시키는 식으로 방어하길래 그 영향을 받은 게 큰 것 같아요!

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

kdkdhoho · 2023-10-10T03:31:10Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+    public static void rollback(final Connection connection) {
+        try {
+            connection.rollback();
+        } catch (final SQLException e) {
+            throw new DataAccessException(e);
+        }
    }


전 rollback 메서드를 DataSourceUtils에 두었어요. 이유는 Connection 객체를 다루는 다른 메서드들과 성격이 비슷하다고 판단했기 때문입니다.
연어는 TransactionSynchronizationManager에 둔 이유가 무엇인가요?

우선 DataSourceUtils에서 getConnection을 할 때 커넥션을 저장해두고, releaseConnection을 할 때 커넥션을 제거하는 상황입니다.
따라서 DataSourceUtils는 해당 DataSource에 대해서 커넥션을 맺고 끊음의 책임이 있다고 생각합니다.
그 커넥션이 뭘 하는지는 관심이 없구요.

그래서 트랜잭션용 커넥션 동기화 책임을 가지는 TransactionSynchronizationManager에 뒀었는데 다시 생각해보니 이 객체의 책임도 아닌 것 같아요! 이 객체의 역할은 DataSource에 대한 Connection Holder라고 생각합니다.

결과적으로 TxUserService로 이동시키는 게 현재 가장 적합하다고 생각해서 옮겼습니다!

따라서 DataSourceUtils는 해당 DataSource에 대해서 커넥션을 맺고 끊음의 책임이 있다고 생각합니다.

완전 납득이 가네요 !
TxUserService에 위치하는 거 완전 동의합니다 👍

kdkdhoho · 2023-10-10T03:44:41Z

...src/main/java/org/springframework/transaction/support/TransactionSynchronizationManager.java

+        connections.remove(key);
+
+        if (connections.isEmpty()) {
+            TransactionSynchronizationManager.resources.remove();
+        }


현재는 각 쓰레드별로 static 변수인 DataSource와, dataSource로 가져온 Connection을 Map으로 묶어서 각 쓰레드에 할당하고 있다고 이해했는데요.
따라서 connections.remove(key) 메서드가 필요한지 의문이 들었어요.
바로 resources.remove()를 호출하면, 현재 진행중인 쓰레드에 할당된 자원이 모두 제거되는 것 아닌가요??

제가 이해를 잘 못한 것 같은데 어떤 부분을 문제로 보시는 지 잘 모르겠어요...!

우선 제 로직의 의도는 아래와 같습니다.
connections.isEmpty()가 true인 상황은 현재 스레드에 저장된 커넥션이 아무것도 없는 상태(커넥션은 존재할 지라도 그에 대한 참조는 없는 상태)입니다!
따라서 혹시나의 오염을 방지하기 위해서 할당돼있는 HashMap을 제거해주도록 했습니다.

다른 스레드에서 어떤 DataSource에 어떤 Connection을 가지고 있던 아무 상관 없습니다!
설령 현재 스레드에서 사용하는 DataSource와 같은 DataSource를 다른 스레드에서 사용하고 있더라두요!

제가 이해했을 때 연어는 ThreadLocal에 HashMap을 할당한 다음에, 그 안에 있는 값들을 다룸으로써 Connection을 처리하시는 것 같아요.
저는 ThreadLocal에 HashMap 자체를 하나의 Connection으로 생각하고 처리했습니다.
서로 생각하는 방식이 약간 다른 것 같아요!

결국, HashMap 자체를 Connection으로 처리해도 되지 않을까? 하는 생각에 리뷰를 남겼습니다.
제가 쓰레드 로컬에 대한 이해도가 부족하거나 틀리게 생각할 수 있으니, 제가 잘못 알고 있는거라면 말씀해주세요..!

추가로 저는 다음과 같이 구현했어요 !!

@Nullable public static Connection getResource(DataSource key) { Map<DataSource, Connection> resource = resources.get(); if (resource == null) { return null; } return resource.get(key); } public static Connection unbindResource(DataSource key) { Connection resource = getResource(key); if (isNull(resource)) { throw new IllegalArgumentException("There is no resource"); } resources.remove(); return resource; }

위에 남겨주신 코멘트들이 전부 이해가 갔어요!

하지만 지금의 ThreadLocal에도 여러 Connection을 저장할 수 있다고 생각해요.
답변 코멘트들로 남겨두긴 했는데 DataSource의 구현체는 여러 개(여러 rdbms를 스프링에서 사용한다면)일 수 있고, 따라서 하나의 서비스 메서드에서 여러 DataSource에 대한 커넥션들을 한 스레드에서 관리할 수 있습니다.

정리하자면 현재 구조로는,
한 DataSource에 대한 여러 Connection은 불가능합니다.
하지만 여러 DataSource를 하나의 트랜잭션에서 사용하는 것은 가능합니다.

만약 도기가 말씀하신 대로의 기능을 가지는 TransactionSynchronizationManager를 구현하시고 싶으시다면 ThreadLocal<Connection>의 타입으로 가지면 될 것 같아요!!

현재는 하나의 DataSource를 사용하기에 '여러 DataSource를 하나의 트랜잭션에서 사용하는 것'은 생각하지 않았던 것 같아요.
그리고 실제로 ThreadLocal<Connection>으로 구현 후 테스트 돌려보니 모두 정상 동작하더라구요 ㅎ

하지만 여러 DataSource를 사용할 수 있다는 가정하엔, 연어가 생각하고 구현하신대로 하는게 더 적절하다고 생각합니다! 👍

...test/java/org/springframework/transaction/support/TransactionSynchronizationManagerTest.java

nuyh99 · 2023-10-10T15:10:52Z

답글들을 달다 보니 제가 반항하는 것 같이 느껴지는데...항상 도기에게 악의는 없습니다ㅠㅠ
꼼꼼한 리뷰에 감사함만 느낄 뿐...🥹

잘 부탁드립니다!!

kdkdhoho · 2023-10-11T05:08:18Z

ㅋㅋㅋㅋㅋ 반항이라뇨 저는 괜찮습니다.
원래 미션하면서 이런 티키타카해야 재밌지 않나요? 저는 재밌던데..
그래서 한 번 더 티키타카 할까합니다! 잘 부탁드려요 😀

nuyh99 · 2023-10-11T09:33:08Z

슛!

kdkdhoho

jdbc 미션 정말 고생많으셨습니다~!~!
연어랑 티키타카하는거 너무 재밌었어요 🎉

리팩터링 미션.. 다들 말 많던데 얼른 우테코 마지막 미션 하러 가시죠 !
정말정말 고생많으셨습니다 !

nuyh99 added 4 commits October 9, 2023 14:50

feat: TransactionSynchronizationManager 구현

932af39

refactor: 커넥션 관리 DataSourceUtils 사용하도록 변경

597e130

refactor: DAO가 현재 스레드의 커넥션을 사용하도록 변경

2d3b0c2

refactor: Application 로직만 있는 객체와 트랜잭션 관련 객체로 분리

422eaac

nuyh99 self-assigned this Oct 9, 2023

refactor: rollback의 역할 TransactionSynchronizationManager에게 위임

995081d

kdkdhoho requested changes Oct 10, 2023

View reviewed changes

nuyh99 added 2 commits October 10, 2023 22:49

refactor: close(), unbindResource() 같이 실행 되도록 변경

ee33f0e

refactor: rollback의 책임 TxUserService에게 위임

ab5398f

kdkdhoho approved these changes Oct 11, 2023

View reviewed changes

kdkdhoho merged commit 16c23df into woowacourse:nuyh99 Oct 11, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 4단계] 연어(황재현) 미션 제출합니다. #577

[JDBC 라이브러리 구현하기 - 4단계] 연어(황재현) 미션 제출합니다. #577

nuyh99 commented Oct 9, 2023

kdkdhoho left a comment

kdkdhoho Oct 10, 2023

nuyh99 Oct 10, 2023

kdkdhoho Oct 11, 2023

nuyh99 Oct 11, 2023

kdkdhoho Oct 11, 2023

kdkdhoho Oct 10, 2023

nuyh99 Oct 10, 2023

kdkdhoho Oct 11, 2023

nuyh99 Oct 11, 2023

kdkdhoho Oct 11, 2023

kdkdhoho Oct 10, 2023

nuyh99 Oct 10, 2023 •

edited

Loading

kdkdhoho Oct 11, 2023

nuyh99 Oct 11, 2023

kdkdhoho Oct 10, 2023

nuyh99 Oct 10, 2023

kdkdhoho Oct 11, 2023

kdkdhoho Oct 10, 2023

nuyh99 Oct 10, 2023

kdkdhoho Oct 11, 2023

kdkdhoho Oct 11, 2023

nuyh99 Oct 11, 2023

kdkdhoho Oct 11, 2023

nuyh99 commented Oct 10, 2023

kdkdhoho commented Oct 11, 2023

nuyh99 commented Oct 11, 2023

kdkdhoho left a comment

[JDBC 라이브러리 구현하기 - 4단계] 연어(황재현) 미션 제출합니다. #577

[JDBC 라이브러리 구현하기 - 4단계] 연어(황재현) 미션 제출합니다. #577

Conversation

nuyh99 commented Oct 9, 2023

구현 목록

kdkdhoho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuyh99 Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuyh99 commented Oct 10, 2023

kdkdhoho commented Oct 11, 2023

nuyh99 commented Oct 11, 2023

kdkdhoho left a comment

Choose a reason for hiding this comment

nuyh99 Oct 10, 2023 •

edited

Loading