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

hectick · 2023-10-09T11:15:51Z

안녕하세요 채채
빨간날이지만 힘내서 코딩을 마저 해보았습니다

3단계때 코멘트 달아주신 부분에 대한 답변은 달아놓았으니 확인해주세용

이번 단계를 진행하면서 모든 요청은 반드시 TxUserService를 거칠 것이라 생각하고 만들었습니다.
왜냐하면 언제는 트랜잭션 없이 바로 AppUserService를 호출하고 언제는 트랜잭션 있게 TxUserService를 호출하고 이것을 현재 코드상으로 어떻게 구분해줄지 모르겠거든요
그래서 jdbcTemplate에는 따로 connection을 close하는 로직을 넣지 않았어요. 어차피 TxUserService의 TransactionExecutor가 해줄테니까용ㅎㅎ,..

그럼 리뷰 잘부탁해용😘

chaewon121

이리내 안녕하세요~! 이번 미션 어려웠는데 이리내는 잘 구현해주셨군요~! 저도 아직 트렌젝션 커넥션처리 등등에 관해서 이해도가 높지않아서 이리내의 의견이 궁금한 점들이 있어서 코멘트로 남겨두었습니다~! 커넥션을 닫는 로직에 대해서 좀더 의견을 나누고 싶네요~!

chaewon121 · 2023-10-10T02:51:19Z

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

 import javax.sql.DataSource;
 import java.sql.Connection;
 import java.util.Map;

 public abstract class TransactionSynchronizationManager {

-    private static final ThreadLocal<Map<DataSource, Connection>> resources = new ThreadLocal<>();
+    private static final ThreadLocal<Map<DataSource, Connection>> resources = ThreadLocal.withInitial(HashMap::new);


withInitial 잘 사용하셨군요!

지선생한테 배웠습니다 ㅋ.ㅋ

chaewon121 · 2023-10-10T03:28:32Z

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

+    @Nullable
    public static Connection getResource(DataSource key) {
-        return null;
+        return resources.get().getOrDefault(key, null);


인자에final을 붙여준다면 코드컨벤션에 더 맞는 코드가 될것같아요!ㅎㅎ

힘내볼게요

chaewon121 · 2023-10-10T03:31:52Z

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

+    @Override
+    public void insert(final User user) {
+        transactionExecutor.execute(() -> userService.insert(user));
+    }


캬 나머니 코드도 모두 트렌젝션을 적용해주셨군요 굳입니다

chaewon121 · 2023-10-10T03:52:29Z

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

@@ -30,6 +30,7 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd
    public static void releaseConnection(Connection connection, DataSource dataSource) {
        try {
            connection.close();
+            TransactionSynchronizationManager.unbindResource(dataSource);


connection.close(); 에서 예외가 발생하더라도
TransactionSynchronizationManager.unbindResource(dataSource);가 일어날 수 있도록 하는것이 좋을까요? 예외가 발생하면 일어나지 않도록 하는것이 좋을까요? 저도 고민이 되었었는데 이리내와 얘기를 나눠보고싶네요!

그러게요 생각해보지 못했는데 고민할만한 내용인 것 같습니다
생각해보니 저라면 unbind 해줄 것 같아요
쓰레드가 재사용되는 상황이라면, 이전 Connection이 비워지지 않고 남아있으면 위험하지 않을까요??

chaewon121 · 2023-10-10T04:51:14Z

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

-            return executeWithConnection(connection, preparedStatementCallback, executionCallback);
-        } catch (SQLException e) {
-            throw new DataAccessException(e);
-        }
-    }
-
-    public int update(final Connection connection, final String sql, final Object... args) {
-        return executeWithConnection(connection, conn -> prepareStatement(sql, conn, args), PreparedStatement::executeUpdate);
-    }
-
-    private <T> T executeWithConnection(final Connection connection,
-                                        final PreparedStatementCallback preparedStatementCallback,
-                                        final ExecutionCallback<T> executionCallback) {
+        final Connection connection = DataSourceUtils.getConnection(dataSource);


만약 트렌젝션 밖에서 해당 함수가 호출이 되었다면 커넥션을 어떻게 닫을 수 있을까요!? 저도 이부분에 대해서 많이 고민해 보았는데요! 만약 트렌젝션이 실행중이라면 닫지 않고, 실행중이 아니라면 커넥션을 닫는 로직에 대해서 이리내의 의견이 궁금합니다!

저도 코드리뷰를 하다가 힌트를 좀 얻었는데요
실제로 ConnectionHolder라는 클래스가 존재하더라구요
Connection 객체를 한번 더 감싸준 클래스인데요 필드에 transactionActive가 있어서 이걸 토대로 트랜잭션이 실행중인지 아닌지 판단해서 커넥션을 닫아줄지 말지를 결정할 수 있을 것 같아요
결론은 Connection을 ConnectionHolder로 한번 더 감싸주고 필드에 transactionActive를 추가하는 것!! 입니다.

chaewon121 · 2023-10-10T06:03:33Z

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

+        resources.get().put(key, value);
    }

    public static Connection unbindResource(DataSource key) {
-        return null;
+        return resources.get().remove(key);


get()의 중복제거에 대해서 이리내는 어떻게 생각하시나요?

get()의 중복 제거가 resources.get()을 따로 메서드로 빼는 것인가요?!
필드가 어떻게 생겼는지 인지를 못했다면(ThreadLocal로 감싸져있는 것을 모른다면) 현재 코드를 봤을 때 resources에서 왜 get을 해와서 지우는거지? 생각할 수도 있을 것 같아요(실제로 제가 그랬음)
getResources().remove(key) 이러면 더 직관적이고 좋은 것 같슴니다

chaewon121

이리내 안녕하세요~! 학습테스트까지 모두 하시느라 고생 많으셨습니다. 저의 지식을 총동원해서 리뷰를 남겼었는데 모두 적용해주셨네요 ..감동받았구요..
이리내의 답변을 보고 저도 힌트를 많이 얻어갈 수 있었네요!
이만 머지하도록 하겠습니다~!

chaewon121 · 2023-10-12T15:58:18Z

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

+            if(connectionHolder.isTransactionActive()) {
+                return;
+            }
+            TransactionSynchronizationManager.unbindResource(dataSource);


저또한 예외가 발생하더라도 비워주는것이 현재코드로서는 안전하다는 생각이 드네요
저같은 경우는 final로 처리를 하였는데 여기 순서를 변경하는 방법으로 구현하셨군요~!이런 방법도 있네요!?

chaewon121 · 2023-10-12T16:01:37Z

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

+        return getResources().getOrDefault(key, null);
    }

    public static void bindResource(final DataSource key, final Connection value) {
-        resources.get().put(key, value);
+        getResources().put(key, value);
    }

    public static Connection unbindResource(final DataSource key) {
-        return resources.get().remove(key);
+        return getResources().remove(key);
+    }
+
+    private static Map<DataSource, Connection> getResources() {
+        return resources.get();


취향차이일수도 있지만 더 가독성이 올라간 코드가 된것같네욯ㅎ

chaewon121 · 2023-10-12T16:05:11Z

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

+
+public class ConnectionHolder {
+
+    private Connection connection;
+    private boolean transactionActive = false;
+
+    public ConnectionHolder(final Connection connection) {
+        this.connection = connection;


코멘트에 남겨주신것처럼 커넥션 홀더를 적용해주셨네요! 감동1

hectick added 3 commits October 9, 2023 19:17

feat: Transaction synchronization 적용

974d28d

feat: 트랜잭션 서비스 추상화

fbc0f6c

feat: 트랜잭션 실행하는 클래스 분리

d08c11f

chaewon121 requested changes Oct 10, 2023

View reviewed changes

chaewon121 reviewed Oct 10, 2023

View reviewed changes

hectick added 5 commits October 10, 2023 17:28

feat: 학습테스트 ...

67417a1

refactor: 파라미터에 final 추가

24ff04a

refactor: 데이터소스 unbind 순서 변경

309ca75

refactor: 메서드 분리

882c2f6

feat: 트랜잭션이 없어도 jdbcTemplate에서 커넥션을 닫을 수 있도록 변경

bd626c9

chaewon121 approved these changes Oct 12, 2023

View reviewed changes

chaewon121 merged commit 53077b2 into woowacourse:hectick Oct 12, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hectick commented Oct 9, 2023 •

edited

Loading

chaewon121 left a comment

chaewon121 Oct 10, 2023

hectick Oct 10, 2023

chaewon121 Oct 10, 2023

hectick Oct 10, 2023

chaewon121 Oct 10, 2023

chaewon121 Oct 10, 2023

hectick Oct 10, 2023 •

edited

Loading

chaewon121 Oct 10, 2023

hectick Oct 10, 2023

chaewon121 Oct 10, 2023

hectick Oct 10, 2023 •

edited

Loading

chaewon121 left a comment

chaewon121 Oct 12, 2023

chaewon121 Oct 12, 2023

chaewon121 Oct 12, 2023

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

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

Conversation

hectick commented Oct 9, 2023 • edited Loading

chaewon121 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

hectick 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

hectick Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

chaewon121 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

hectick commented Oct 9, 2023 •

edited

Loading

hectick Oct 10, 2023 •

edited

Loading

hectick Oct 10, 2023 •

edited

Loading