[JDBC 라이브러리 구현하기 - 4단계] 이레(이다형) 미션 제출합니다. #553

zillionme · 2023-10-09T03:21:10Z

안녕하세요 저문! 이레입니다

리뷰를 받고 저문의 리뷰를 적용하면서, 전부터 생각해온 내용을 추가해서 리팩터링했는데요.

1. 커넥션 객체를 ConnectionHolder 이라는 래퍼 클래스로 감쌌습니다.

그 이유는 다음과 같습니다.
JdbcTemplate에서 DataSourceUtils를 통해 커넥션을 해제할 때, 트랜잭션에 속한 커넥션이라면 커넥션이 close되서는 안됩니다.
띠라서, 커넥션은 트랜잭션에 속해있는가 아닌가라는 상태를 가지고 있어야 하기때문에, ConnectionHolder 이라는 래퍼 클래스로 감싸야합니다.
=> 즉, DataSourceUtils.releaseConnection()에서 커넥션이 트랜잭션에 속해있다면(트랜잭션Active하다면) connection.close()하지 않고,
커넥션이 트랜잭션에 속해있지 않다면(트랜잭션Inactive하다면) connection.close()합니다.

2. 커넥션의 트랜잭션의 시작과 종료는 트랜잭션 매니저의 책임으로 코드를 작성했습니다.

트랜잭션 매니저에서

DataSourceUtils.getConnection()을 실행한 후, ConnectionHolder를 통해 트랜잭션의 상태를 active true로 바꿔줍니다.
커밋 혹은 롤백 후, ConnectionHolder를 통해 트랜잭션의 상태를 active true로 바꿔줍니다.
DataSourceUtils.releaseConnection()을 호출할 때, 커넥션의 트랜잭션 상태가 false여야 하기 때문입니다.

이를 통해, JdbcTemplate에서 DataSourceUtils.getConnection()을 실핼해, 새로 커넥션을 생성한다면,
다시말해, 트랜잭션 매니저를 통해 커넥션이 스레드로컬에 저장된 상태가 아니라면, 트랜잭션의 상태가 active false인(=트랜잭션에 속하지 않은) 커넥션을 반환합니다.
DataSourceUtils에서는 트랜잭션 시작과 종료를 관리하지 않고, 단순히 스레드 로컬에 저장된 커넥션을 가져오는 책임만 있기 때문입니다.

3. 저문의 마지막 리뷰에 대해서는 제대로 이해를 하지못했습니다.

다시 리뷰 주시면 해당 내용 적용하도록 하겠습니다.

jeomxon

안녕하세요 이레 :)
늦은 시간까지도 열심히 구현해주셨네요👍🏻
반영해주셨으면 하는 부분이 있어서 Request Changes로 남겨두겠습니다!

이해 안되시는 부분이 있다면 언제든 말씀해주세요 :)
마지막까지 파이팅입니다 🔥

jeomxon · 2023-10-09T05:59:41Z

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

@@ -29,6 +29,8 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd

    public static void releaseConnection(Connection connection, DataSource dataSource) {
        try {
+            TransactionSynchronizationManager.unbindResource(dataSource);
+            connection.setAutoCommit(true);


이건 정말 몰라서 질문드립니다..!
커넥션을 close할 때 setAutoCommit을 true로 설정해줘야하는 이유가 있나요?

build.gradle파일을 확인 해보니, jdbc를 h2를 사용해서 사실 해당 내용은 필요가 없겠네요.
커넥션 풀을 사용하면, 커넥션을 쓰고 반환할 때, 다음 쓰레드에서 트랜잭션을 사용하지 않을 수도 있기 때문에 autoCommit을 다시 true로 바꿔줍니다!

jeomxon · 2023-10-09T06:05:17Z

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

@@ -1,6 +1,6 @@
 package org.springframework.jdbc.datasource;

-import org.springframework.jdbc.CannotGetJdbcConnectionException;
+import org.springframework.jdbc.exception.CannotGetJdbcConnectionException;


이제 이 클래스는 삭제해도 될 것 같네요 ㅎㅎ

삭제했습니다!

jeomxon · 2023-10-09T06:21:58Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionManager.java

        }
    }

-private void rollback(final Connection conn) {
+    private void rollback(final Connection conn) {


현재는 롤백에 대해서만 메서드 분리를 해주셨는데요.
commit()이나 setAutoCommit()에 대해서는 따로 분리를 하지 않으신 이유가 있을까요?

commit()이나 setAutoCommit()에서 발생할 수 있는 SQL예외는 try-catch문으로 처리하지만
rollback()같은 경우에 새로운 SQL예외가 발생하기 때문에 메서드 분리를 해주었습니다.
commit()이나 setAutoCommit()을 각각 메서드 분리하면 각각 try-catch문으로 처리해주거나 SQL예외를 던지도록 해야하는데 의미가 없을 것 같아서요.

저문은 메서드 분리를 하셨나요?

setAutoCommit, commit, rollback에서 발생하는 SQLException은
DAO에서 발생하는 SQLException과는 성격이 조금 다르다고 생각했습니다!
따라서 저같은 경우는 해당 메서드들에 대한 메서드 분리를 하고 try-catch문을 통해서 커스텀 예외를 던지도록 변경해줬어요.

해당 내용에 대해서는 아래와 같이 수정했습니다.
이유는 저문의 리뷰처럼 Jdbc템플릿에서 발생하는 sql예외라는 성격이 다르지만, 동시에 commit rollback 모두 트랜잭션 관련 예외기 때문입니다.

public <T> T executeWithResult(final Supplier<T> logicExecutor) { final Connection conn = DataSourceUtils.getConnection(dataSource); setTransactionActive(conn); try { conn.setAutoCommit(false); T result = logicExecutor.get(); conn.commit(); return result; } catch (Exception e) { rollback(conn); throw new TransactionException(e); } finally { setTransactionInactive(conn); DataSourceUtils.releaseConnection(conn, dataSource); } }

private void rollback(final Connection conn) { try { conn.rollback(); } catch (SQLException e) { throw new TransactionException(e); } }

jeomxon · 2023-10-09T06:28:38Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionManager.java

+        });
+    }
+
+    public <T>T executeWithResult(final Supplier<T> logicExecutor) {


반환타입이 있는 것과 없는 것 두개로 나눠서 만들어주셨군요!
혹시 애플리케이션 코드에서 null을 반환하는 것 때문에 이렇게 설계하신걸까요?

그리고 execute()는 파라미터로 LogicExecutor를 직접 만들어서 사용해주셨고,
executeWithResult()는 파라미터로 Supplier를 사용해주셨는데, 이유가 있을까요?

깊게 고민하지 않고 기존의 코드를 사용했습니다..ㅎㅎ

LogicExecutor는 실제로는 Runnable 함수형 인터페이스인데요.
코드의 일관서을 위해 Runnable로 바꾸는 것이 맞겠지요?

코드의 일관성을 위해 Runnable로 바꾸도록 하겠습니다!

네 이 부분은 코드의 일관성 측면에서 말씀 드린 것이었어요.
어떤 방식을 사용하든 잘 동작하겠지만 일관성만 있게 사용한다면 둘 다 괜찮을 것 같았습니다!

jeomxon · 2023-10-09T06:36:12Z

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

+        Map<DataSource, Connection> map = resources.get();
+        if (map == null || !map.containsKey(key)) {
+            return null;
+        }
+        return map.remove(key);


저도 처음에는 그냥 넘어갔지만, 지토에게 리뷰를 받고 알게 된 부분이 있어서 말씀 드립니다!
현재는 unbind를 해줄 때 단순히 map에서 값만 지워주고 있는데요.
그렇다면 ThreadLocal에서 쓰레드의 상태인 Map<DataSource, Connection>은 어떻게 처리하면 좋을까요?

map자체도 삭제해야할까요? 이 질문에 대해서 정확한 이해를 하지 못했기 때문에 한번더 답변주시면 감사하겠습니다!(이 리뷰에 대해서는 차후에 반영하도록 할게요)

ThreadLocal<Map<DataSource, Connection>>형태로 가지고 있는데,
현재는 map이 들고 있는 값만 remove해주는 것으로 보여요.
map이 빈 상태라면 ThreadLocal에서 map도 지워주는 것이 좋을 것 같아서 남긴 리뷰였습니다.

zillionme · 2023-10-10T12:29:50Z

안녕하세요 저문! 이레입니다

리뷰를 받고 저문의 리뷰를 적용하면서, 전부터 생각해온 내용을 추가해서 리팩터링했는데요.

1. 커넥션 객체를 ConnectionHolder 이라는 래퍼 클래스로 감쌌습니다.

그 이유는 다음과 같습니다.
JdbcTemplate에서 DataSourceUtils를 통해 커넥션을 해제할 때, 트랜잭션에 속한 커넥션이라면 커넥션이 close되서는 안됩니다.
띠라서, 커넥션은 트랜잭션에 속해있는가 아닌가라는 상태를 가지고 있어야 하기때문에, ConnectionHolder 이라는 래퍼 클래스로 감싸야합니다.
=> 즉, DataSourceUtils.releaseConnection()에서 커넥션이 트랜잭션에 속해있다면(트랜잭션Active하다면) connection.close()하지 않고,
커넥션이 트랜잭션에 속해있지 않다면(트랜잭션Inactive하다면) connection.close()합니다.

2. 커넥션의 트랜잭션의 시작과 종료는 트랜잭션 매니저의 책임으로 코드를 작성했습니다.

트랜잭션 매니저에서

DataSourceUtils.getConnection()을 실행한 후, ConnectionHolder를 통해 트랜잭션의 상태를 active true로 바꿔줍니다.
커밋 혹은 롤백 후, ConnectionHolder를 통해 트랜잭션의 상태를 active true로 바꿔줍니다.
DataSourceUtils.releaseConnection()을 호출할 때, 커넥션의 트랜잭션 상태가 false여야 하기 때문입니다.

이를 통해, JdbcTemplate에서 DataSourceUtils.getConnection()을 실핼해, 새로 커넥션을 생성한다면,
다시말해, 트랜잭션 매니저를 통해 커넥션이 스레드로컬에 저장된 상태가 아니라면, 트랜잭션의 상태가 active false인(=트랜잭션에 속하지 않은) 커넥션을 반환합니다.
DataSourceUtils에서는 트랜잭션 시작과 종료를 관리하지 않고, 단순히 스레드 로컬에 저장된 커넥션을 가져오는 책임만 있기 때문입니다.

3. 저문의 마지막 리뷰에 대해서는 제대로 이해를 하지못했습니다.

다시 리뷰 주시면 해당 내용 적용하도록 하겠습니다.

jeomxon · 2023-10-10T15:58:27Z

jdbc/src/main/java/org/springframework/transaction/support/ConnectionHolder.java

+
+    private final Connection connection;
+
+    private boolean isConnectionTransactionActive;


생성자에서도 따로 값을 받지 않고 있는데, default값을 설정해주는 것이 어떨까요?
지금과 같은 경우 setter를 사용하지 않으면 값이 할당되지 않을 것 같아요.

boolean은 기본형이기 때문에 false가 기본값으로 설정되는 것으로 알고 있습니다!

엇 그러네요.. 죄송합니다..ㅎㅎ

제안드리고 싶었던 점은 기본 값을 false로 명시해두고,
setter(false)를 이용하지 않는 방법이 더 좋아보여서 말씀드렸습니다!

앗 그렇게 수정하겠숩니다!

jeomxon · 2023-10-10T16:08:11Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionManager.java

+    private void setTransactionActive(final Connection conn) {
+        ConnectionHolder connectionHolder = new ConnectionHolder(conn);
+        connectionHolder.setConnectionTransactionActive(true);
+        TransactionSynchronizationManager.bindResource(dataSource, connectionHolder);
+    }
+
+    private void setTransactionInactive(final Connection conn) {
+        ConnectionHolder connectionHolder = new ConnectionHolder(conn);
+        connectionHolder.setConnectionTransactionActive(false);
+        TransactionSynchronizationManager.bindResource(dataSource, connectionHolder);
+    }


이 부분은 boolean을 파라미터로 빼서 하나의 메서드로 관리하는 것이 좋아보여요!

네이밍을 통해 좀 더 가독성이 좋은 코드를 작성하려고 했던것인데, 중복코드가 발생하기 때문에 저문의 리뷰대로 수정하겠습니다

jeomxon · 2023-10-10T16:11:43Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+        } finally {
+            DataSourceUtils.releaseConnection(conn, dataSource);


여기서 커넥션을 close하는 로직을 추가해주셨는데 이유가 있을까요?
제가 생각했을 때 JdbcTemplate에서는 커넥션을 가져오기는 하지만,
close할 때는 TransactionManager클래스의 executeWithResult메서드가 끝나는 시점에 해준다고 이해를 했어요.
따라서 결국 비즈니스 로직이 실행되고 마지막에 단 한번만 호출되면 된다고 생각했습니다.

만약 트랜잭션이 없는 커넥션의 경우에는 (= 서비스에서 트랜잭션 매니저 클래스를 사용하지 않고, 매번 새로운 커넥션을 생성하는 방식으로 쿼리작업이 이뤄지는 경우)
커넥션이 해제되지 않기 때문입니다.

위와 같이 작업할 경우,
JdbcTemplate에서 커넥션을 생성하는 것(트랜잭션이 없는 커넥션 = 커넥션 홀더 내의 커넥션의 트랜잭션활성화 상태가 false)과
TransactionManager에서 커넥션을 생성하는 것(트랜잭션이 있는 커넥션 = 커넥션 홀더 내의 커넥션의 트랜잭션활성화 상태가 true)이 분리됩니다.

jeomxon · 2023-10-11T02:13:56Z

jdbc/src/main/java/org/springframework/transaction/support/TransactionManager.java

        }
    }

-private void rollback(final Connection conn) {
+    private void rollback(final Connection conn) {


setAutoCommit, commit, rollback에서 발생하는 SQLException은
DAO에서 발생하는 SQLException과는 성격이 조금 다르다고 생각했습니다!
따라서 저같은 경우는 해당 메서드들에 대한 메서드 분리를 하고 try-catch문을 통해서 커스텀 예외를 던지도록 변경해줬어요.

jeomxon

마지막까지 드린 리뷰들에 대한 의견을 나눌 수 있어서 좋았습니다!
충분한 대화를 나눈 것 같아서 리뷰하는 동안 즐거웠어요.
덕분에 저도 정말 많이 배웠고, 유익한 시간을 보냈습니다 :)

긴 여정 끝에 마지막 미션만 남았네요..!
환절기 건강 조심하시고, 마지막까지 파이팅입니다!🔥🔥

zillionme added 10 commits October 9, 2023 02:30

feat: TransactionSynchronizationManager 리소스 bind, unbind, get 기능 추가

f82dc21

feat: 커넥션 해제 시, 쓰레드 로컬에 저장된 커넥션 제거

be773f8

refactor: 쓰레드 로컬을 통한 커넥션 동기화 적용

db74c8e

refactor: UserService 인터페이스 생성

d6af471

feat: 결과 있는 트랜잭션 실행 기능 추가

a308b22

feat: AppUserService의 트랜잭션 프록시 클래스 추가

cbeda5a

test: 테스트 수정

df0bddc

chore: 예외 클래스 패키지 이동

b8bcbe5

test: aop 테스트

3b7869f

chore: final 키워드 수정 및 필요없는 메서드 지우기

9470118

jeomxon requested changes Oct 9, 2023

View reviewed changes

zillionme added 4 commits October 10, 2023 21:07

chore: 필요없는 클래스 삭제

a7b16bb

refactor: 커넥션의 트랜잭션 active 상태에 따른 커넥션 해제 방법 수정

046b3c8

refactor: TransactionManager 트랜잭션 시작과 종료 관리 기능 추가

8cf6bda

refactor: JdbcTemplate 커넥션 해제 기능 추가

2bfef4b

jeomxon reviewed Oct 11, 2023

View reviewed changes

zillionme added 5 commits October 11, 2023 14:17

fix: 트랜잭션 동기화 매니저 unbind에 스레드 로컬에 대한 자원 해제 기능 추가

8cf2002

refactor: 트랜잭션 매니저에서 발생하는 SQL예외 처리방법 수정

237a3a3

refactor: 중복 코드 제거

6d05798

chore: 주석 제거

b2a6103

refaco: 트랜잭션 동기화 매니저 unbind에 스레드 로컬에 대한 자원 해제 기능 추가

22da3a4

jeomxon approved these changes Oct 11, 2023

View reviewed changes

jeomxon merged commit e47ece9 into woowacourse:zillionme Oct 11, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 4단계] 이레(이다형) 미션 제출합니다. #553

[JDBC 라이브러리 구현하기 - 4단계] 이레(이다형) 미션 제출합니다. #553

zillionme commented Oct 9, 2023 •

edited

Loading

jeomxon left a comment

jeomxon Oct 9, 2023

zillionme Oct 10, 2023

jeomxon Oct 9, 2023

zillionme Oct 10, 2023

jeomxon Oct 9, 2023

zillionme Oct 10, 2023

jeomxon Oct 11, 2023

zillionme Oct 11, 2023 •

edited

Loading

jeomxon Oct 9, 2023

zillionme Oct 10, 2023

zillionme Oct 10, 2023

jeomxon Oct 10, 2023

jeomxon Oct 9, 2023

zillionme Oct 10, 2023

jeomxon Oct 10, 2023 •

edited

Loading

zillionme commented Oct 10, 2023

jeomxon Oct 10, 2023

zillionme Oct 11, 2023

jeomxon Oct 11, 2023

zillionme Oct 11, 2023

jeomxon Oct 10, 2023

zillionme Oct 11, 2023

jeomxon Oct 10, 2023

zillionme Oct 11, 2023 •

edited

Loading

jeomxon Oct 11, 2023

jeomxon left a comment


		private final Connection connection;

		private boolean isConnectionTransactionActive;

		} finally {
		DataSourceUtils.releaseConnection(conn, dataSource);

[JDBC 라이브러리 구현하기 - 4단계] 이레(이다형) 미션 제출합니다. #553

[JDBC 라이브러리 구현하기 - 4단계] 이레(이다형) 미션 제출합니다. #553

Conversation

zillionme commented Oct 9, 2023 • edited Loading

1. 커넥션 객체를 ConnectionHolder 이라는 래퍼 클래스로 감쌌습니다.

2. 커넥션의 트랜잭션의 시작과 종료는 트랜잭션 매니저의 책임으로 코드를 작성했습니다.

3. 저문의 마지막 리뷰에 대해서는 제대로 이해를 하지못했습니다.

jeomxon 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

zillionme Oct 11, 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

jeomxon Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

zillionme commented Oct 10, 2023

1. 커넥션 객체를 ConnectionHolder 이라는 래퍼 클래스로 감쌌습니다.

2. 커넥션의 트랜잭션의 시작과 종료는 트랜잭션 매니저의 책임으로 코드를 작성했습니다.

3. 저문의 마지막 리뷰에 대해서는 제대로 이해를 하지못했습니다.

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

zillionme Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeomxon left a comment

Choose a reason for hiding this comment

zillionme commented Oct 9, 2023 •

edited

Loading

zillionme Oct 11, 2023 •

edited

Loading

jeomxon Oct 10, 2023 •

edited

Loading

zillionme Oct 11, 2023 •

edited

Loading