[JDBC 라이브러리 구현하기 - 4단계] 성하(김성훈) 미션 제출합니다. #489

sh111-coder · 2023-10-07T08:40:19Z

안녕하세요 루쿠! 빠르게 리뷰해주셔서 4단계 구현할 수 있었어요!! 👍🏻👍🏻

이번에는 미션 요구사항에 나온대로 구현을 진행해봤어요!

Connection을 파라미터로 받는 서비스의 로직들을 제거하고, DataSourceUtil & TransactionSynchronizationManager을 사용하여 Connection 동기화
트랜잭션 서비스 추상화

UserService -> Impl : AppUserService / TxUserService
TxUserService에서 트랜잭션 처리 로직 중복 -> TransactionExecutor을 사용하여 Service 로직을 함수형 인터페이스로 받아서 처리
- Return Type 존재 여부에 따라서 execute 메소드 오버로딩해서 함수형 인터페이스를 다르게 받아서 처리

aiaiaiai1

안녕하세요 성하~
4단계 빠르게 구현해주셨네요!
몇가지 의견과 코멘트 남겨드렸는데 확인부탁드려요~

aiaiaiai1 · 2023-10-08T09:41:21Z

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

    public static Connection getResource(DataSource key) {
-        return null;
+        if (resources.get() == null) {
+            return null;
+        }
+        Map<DataSource, Connection> connectionMap = resources.get();
+        return connectionMap.get(key);


맨 첫줄에서 Map<DataSource, Connection> connectionMap = resources.get(); 를 실행하고
if 문에서 connectionMap의 값이 null인지를 확인하면 가독성이 더 좋아질거같아요!

확실히 그렇네요!! 수정했습니다!!

aiaiaiai1 · 2023-10-08T09:56:02Z

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

    public static void bindResource(DataSource key, Connection value) {
+        Map<DataSource, Connection> connectionMap = new HashMap<>();
+        Connection mappedConnection = connectionMap.put(key, value);
+        if (mappedConnection != null) {
+            throw new IllegalStateException("fail to bind resource because of already exist bound thread");
+        }
+        resources.set(connectionMap);
    }


bind 할때마다 새로운 map을 만들어서 resources에 set 해주면은 기존에 resources에 존재했던 DataSource에 대한 Connection의 map 정보가 사라지게 될거같아요!

그리고 if문을 통해 mappedConnection 의 값이 존재하면 예외를 터트리고 있는데
bind 할 Connection은 매번 값이 존재해서 예외가 무조건 발생하게 되지 않나요?! 성하의 의도가 궁금합니다!

bind 할때마다 새로운 map을 만들어서 resources에 set 해주면은 기존에 resources에 존재했던 DataSource에 대한 Connection의 map 정보가 사라지게 될거같아요!

해당 부분은 수정했습니다!!

그리고 if문을 통해 mappedConnection 의 값이 존재하면 예외를 터트리고 있는데
bind 할 Connection은 매번 값이 존재해서 예외가 무조건 발생하게 되지 않나요?! 성하의 의도가 궁금합니다!

이 부분은 Connection mappedConnection = connectionMap.put(key, value);에서 Map의 put의 값을 받고 있는데요!
Map에서 put 메소드의 리턴 값의 의미는 다음과 같습니다!

the previous value associated with key, or null if there was no mapping for key. (A null return can also indicate that the map previously associated null with key, if the implementation supports null values.)

요약하면, 이전에 맵에 key에 해당하는 값이 존재한다면 해당 값을 반환하고, 존재하지 않았다면 null을 반환합니다!
따라서 mappedConnection이 null이 아니라면 이미 스레드에 리소스가 bind 되어 있다는 것을 의미하므로
예외를 던져주게 구현했습니다!!

그래서 put을 하기 전에 이미 존재하는 Connection이 존재한다면 예외를 발생시키도록 했습니다!

아하 그렇군요! 알려주셔서 감사합니다~

aiaiaiai1 · 2023-10-08T10:27:56Z

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

    public static Connection unbindResource(DataSource key) {
-        return null;
+        Map<DataSource, Connection> connectionMap = resources.get();
+        Connection connectionToRemove = connectionMap.get(key);
+        resources.remove();
+        return connectionToRemove;
    }
 }


여기서도 connectionMap의 사이즈가 0일때 resources.remove() 하는게 더 좋아보입니다!

넵!!! 그렇네요 ㅠㅠ
수정했습니다!!

aiaiaiai1 · 2023-10-08T13:07:44Z

jdbc/src/main/java/org/springframework/transaction/TransactionExecutor.java

+    public void execute(final ServiceExecutor serviceExecutor, final boolean isReadOnly) {
+        Connection connection = DataSourceUtils.getConnection(dataSource);
+
+        try {
+            connection.setAutoCommit(false);
+            connection.setReadOnly(isReadOnly);
+            serviceExecutor.doInAction();
+
+            connection.commit();
+        } catch (SQLException e) {
+            try {
+                connection.rollback();
+            } catch (SQLException ex) {
+                throw new DataAccessException(ex);
+            }
+        } finally {
+            DataSourceUtils.releaseConnection(connection, dataSource);
+            TransactionSynchronizationManager.unbindResource(dataSource);
+        }
+    }
+
+    public <T> T execute(final ServiceCallback<T> serviceExecutor, boolean isReadOnly) {
+        Connection connection = DataSourceUtils.getConnection(dataSource);
+
+        try {
+            connection.setAutoCommit(false);
+            connection.setReadOnly(isReadOnly);
+
+            T result = serviceExecutor.doInAction();
+
+            connection.commit();
+            return result;
+        } catch (SQLException e) {
+            try {
+                connection.rollback();
+            } catch (SQLException ex) {
+                throw new DataAccessException(ex);
+            }
+        } finally {
+            DataSourceUtils.releaseConnection(connection, dataSource);
+            TransactionSynchronizationManager.unbindResource(dataSource);
+        }
+        return null;
+    }


반환타입이 있는것과 없는 것으로 메서드를 나누어주었네요!
중복을 제거하는 다른 방법은 없을까요?? 🤔
TxUserService에서 void 메서드를 람다식으로 넘겨서 호출할때
람다식에 return null을 같이 넘겨주어 <T> T execute 한개의 메서드만 사용하는건 어떤것 같나요??!

개선 방법 제안까지 해주셔서 감사합니다!! 😊
중복이 발생하긴 하지만, null을 넘겨주는 것 보다는 오버로딩이 개인적으로 더 좋은 것 같아서 이대로 놔두려고 하는데 괜찮을까요?!

넵 좋습니다 👍

aiaiaiai1 · 2023-10-08T13:10:46Z

jdbc/src/main/java/org/springframework/transaction/TransactionExecutor.java

+        this.dataSource = dataSource;
+    }
+
+    public void execute(final ServiceExecutor serviceExecutor, final boolean isReadOnly) {


readOnly 옵션도 추가해주셨네요! 👍

aiaiaiai1 · 2023-10-08T13:14:22Z

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

    }

    public static Connection unbindResource(DataSource key) {
-        return null;
+        Map<DataSource, Connection> connectionMap = resources.get();
+        Connection connectionToRemove = connectionMap.get(key);


connectionMap이 null 일 수 도 있을거같네용

aiaiaiai1 · 2023-10-08T13:24:00Z

jdbc/src/main/java/org/springframework/transaction/TransactionExecutor.java

+            DataSourceUtils.releaseConnection(connection, dataSource);
+            TransactionSynchronizationManager.unbindResource(dataSource);


개인적으로 dataSource에 대한 connection을 unbind하고 connection을 close하는 순서가 적절하다고 느껴지는데
메서드 순서를 변경해주는건 어떤가요?!

아!! 순서 변경하는게 좋은 것 같아요!!! 수정했습니당! 👍🏻👍🏻

aiaiaiai1 · 2023-10-08T13:47:31Z

app/src/main/java/com/techcourse/service/TxUserService.java

+public class TxUserService implements UserService {
+
+    private final UserService userService;
+    private final TransactionExecutor transactionExecutor;
+
+    public TxUserService(final UserService userService) {
+        this.userService = userService;
+        this.transactionExecutor = new TransactionExecutor(DataSourceConfig.getInstance());
+    }


생성자 파라미터로 혹여나 TxUserService와 같은 의도하지 않은 UserService 구현체가 들어갈수도 있을거같아요
좀더 구체적인 파라미터로 제한해주는건 어떤것같나요??

좋은 것 같아요!! 수정했습니다! 👍🏻👍🏻

sh111-coder · 2023-10-09T06:35:51Z

리뷰 감사합니다 루쿠!! 좀 급하게 4단계 작성해서 TransactionSynchronizationManager 구현 부분에
허점이 좀 많았던 것 같아요!!
남겨주신 코멘트 기반으로 리팩토링 진행했습니다!!

aiaiaiai1

안녕하세요 성하! 피드백 반영해주신거 확인했어요!
다만 한가지 걸리는 부분이 있는거 같아서 RC 보내봅니다..!
다음 요청때 바로 머지하도록 할께요~

aiaiaiai1 · 2023-10-10T04:57:02Z

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

        Connection mappedConnection = connectionMap.put(key, value);
        if (mappedConnection != null) {
            throw new IllegalStateException("fail to bind resource because of already exist bound thread");
        }


현재 connectinMap에서 값을 넣고 올바르지 않으면 예외를 터트려주고있지만 잘못된 값은 map에 들어가게 되는 같아요
그래서 값을 넣기전에 먼저 검증을해주는건 어떨까요??

해당 부분은 넣은 값이 올바르지 않으면 예외를 터트려주는 것이 아니라
넣은 값이 이미 존재한다면 예외를 터트려주는 것으로 이해하시면 될 것 같아요!

Connection mappedConnection = connectionMap.put(key, value);에서 나오면
mappedConnection은 해당 key에 해당하는 value가 존재한다면 존재하는 value를 반환하고,
key에 해당하는 value가 없다면 Map에 key, value를 넣고 null이 들어가게 됩니다!

그래서 예외를 발생시키는 이유가, 이미 ConnectionMap에 DataSource에 해당하는 Connection이 존재한다면
이미 스레드에 bind된 Resource가 있는 것이므로 예외를 발생시켰습니다!

그래서 결론적으로 put을 한 결과가 if 검증에서 필요해서 순서는 저대로 해야하는 것 같아요!

aiaiaiai1

안녕하세요 성하!
피드백 반영하시느라 고생하셨어요 ㅎㅎ
미션 여기서 마무리할께요! 다음 미션도 화이팅입니다~ 🚀

sh111-coder added 5 commits October 7, 2023 15:15

refactor: Connection 파라미터로 받는 로직 삭제

781cc9b

feat: TransactionSynchronizationManager 메소드 구현하여 UserService 리팩토링

be7d4c9

test: findById 테스트 추가

c2492f3

test: insert 테스트 추가

74682d5

feat: 트랜잭션 동기화 & 트랜잭션 서비스 추상화 구현

e5c6b0d

sh111-coder requested a review from aiaiaiai1 October 7, 2023 08:40

sh111-coder self-assigned this Oct 7, 2023

aiaiaiai1 requested changes Oct 8, 2023

View reviewed changes

sh111-coder added 5 commits October 9, 2023 14:05

refactor: 가독성 리팩토링

797af5a

refactor: bindResource, unbindResource 리팩토링

01be39f

refactor: Resource unbind가 먼저 되도록 리팩토링

90875bf

refactor: 파라미터 네이밍 변경

a876ec6

refactor: 생성자 파라미터 구체화

ae6ee43

refactor: unbindResource 책임 DatasourceUtils로 위임

9ea5497

aiaiaiai1 requested changes Oct 10, 2023

View reviewed changes

refactor: 이미 존재하는 Key일 경우 먼저 예외 발생하도록 리팩토링

0cde77e

aiaiaiai1 approved these changes Oct 11, 2023

View reviewed changes

aiaiaiai1 merged commit edbfac6 into woowacourse:sh111-coder Oct 11, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 4단계] 성하(김성훈) 미션 제출합니다. #489

[JDBC 라이브러리 구현하기 - 4단계] 성하(김성훈) 미션 제출합니다. #489

sh111-coder commented Oct 7, 2023

aiaiaiai1 left a comment

aiaiaiai1 Oct 8, 2023

sh111-coder Oct 9, 2023

aiaiaiai1 Oct 8, 2023

sh111-coder Oct 9, 2023

aiaiaiai1 Oct 10, 2023

aiaiaiai1 Oct 8, 2023

sh111-coder Oct 9, 2023

aiaiaiai1 Oct 8, 2023

sh111-coder Oct 9, 2023 •

edited

Loading

aiaiaiai1 Oct 10, 2023

aiaiaiai1 Oct 8, 2023

aiaiaiai1 Oct 8, 2023

aiaiaiai1 Oct 8, 2023

sh111-coder Oct 9, 2023

aiaiaiai1 Oct 8, 2023

sh111-coder Oct 9, 2023

sh111-coder commented Oct 9, 2023

aiaiaiai1 left a comment

aiaiaiai1 Oct 10, 2023

sh111-coder Oct 10, 2023

aiaiaiai1 left a comment

		DataSourceUtils.releaseConnection(connection, dataSource);
		TransactionSynchronizationManager.unbindResource(dataSource);

[JDBC 라이브러리 구현하기 - 4단계] 성하(김성훈) 미션 제출합니다. #489

[JDBC 라이브러리 구현하기 - 4단계] 성하(김성훈) 미션 제출합니다. #489

Conversation

sh111-coder commented Oct 7, 2023

aiaiaiai1 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

sh111-coder Oct 9, 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

sh111-coder commented Oct 9, 2023

aiaiaiai1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aiaiaiai1 left a comment

Choose a reason for hiding this comment

sh111-coder Oct 9, 2023 •

edited

Loading