[JDBC 라이브러리 구현 4단계] 메리(최승원) 미션 제출합니다 #584
Conversation
# Conflicts: # jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
| TransactionSynchronizationManager.startTransaction(); | ||
|
|
||
| userService.changePassword(id, newPassword, createBy); | ||
|
|
||
| TransactionSynchronizationManager.finishTransaction(); |
| private static final ThreadLocal<Map<DataSource, Connection>> resources = ThreadLocal.withInitial(HashMap::new); | ||
| private static final ThreadLocal<Boolean> isActive = ThreadLocal.withInitial(() -> false); |
| public static Connection getResource(DataSource dataSource) throws SQLException { | ||
| if (resources.get().containsKey(dataSource)) { | ||
| return resources.get().get(dataSource); | ||
| } | ||
| final Connection connection = dataSource.getConnection(); | ||
| bindResource(dataSource, connection); | ||
| return connection; |
There was a problem hiding this comment.
P4.
제공된 코드에서 DataSourceUtils의 getConnection 로직을 최대한 이 클래스에 응집시키신 것 같은데,
어떤 의도인지 궁금합니다 !
이로 인해 JdbcTemplate에서도 DataSourceUtils 대신 이 클래스를 통해 connection을 얻고 있는 것을 보았는데요.
추가로 JdbcTemplate도 이 클래스를 알아야 하는가? 트랜잭션에 대해 알아야 하는가? 에 대해서도 같이 생각해보면 좋을 것 같아요 😀
There was a problem hiding this comment.
해당 클래스는 connection을 관리하고 있는 주체이기 때문에 connection에 관한 모든 로직이 TransactionSynchronization에서 이루어져야 한다고 생각했던 것 같습니다! 그런데 너무 관리하는 대상에만 집중하고, 클래스의 역할에 대해서는 고려하지 않았던 것 같습니다. 그래서 해당 클래스가 너무 비대해졌던 것 같아요!
말씀해주신 것처럼 JdbcTemplate이 트랜잭션을 동기화시키는 클래스에 대해 알 필요가 없다고 생각했습니다.
TransactionManager 클래스를 별도로 생성해 해결해주었습니다.
따라서 commit과 rollback, 커넥션을 생성하고 닫아주는 역할 또한 트랜잭션 관리의 일부라고 생각해 TransactionManager로 이동해주었습니다. 이렇게 리팩토링을 진행하니 TransactionSynchronization 클래스는 단순히 같은 트랜잭션이 유지되는 동안 DataSource에 매핑되는 동일한 커넥션을 사용하고 있는지에만 집중할 수 있었던 것 같아요!
|
|
||
| public static Connection getConnection(DataSource dataSource) throws CannotGetJdbcConnectionException, SQLException { |
There was a problem hiding this comment.
급하게 구현하느라 final을 통일 시키지 못하고 제출해버렸네요... 😭
반영했습니다!
| public static void startTransaction() { | ||
| isActive.set(true); | ||
| } | ||
|
|
||
| public static void finishTransaction() { | ||
| isActive.set(false); | ||
| } | ||
|
|
||
| public static <T> T commit(final ExecuteQueryCallback<T> callBack, final PreparedStatement preparedStatement, final DataSource dataSource) { | ||
| try { | ||
| final Connection connection = DataSourceUtils.getConnection(dataSource); | ||
| connection.setAutoCommit(false); | ||
|
|
||
| final T result = callBack.execute(preparedStatement); | ||
|
|
||
| commitTransaction(dataSource, connection); | ||
|
|
||
| return result; | ||
| } catch (SQLException e) { | ||
| throw new CannotGetJdbcConnectionException("jdbc 연결에 실패했습니다."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
P3. 커밋, 롤백 메서드를 별도로 정의해주신 것 너무 좋습니다...🥹 저는 아직 그렇게 하지 못했거든요 ㅎㅎ ㅜㅜ 배워갑니당
(디엠으로 말씀해주신 것처럼, 이미 인지하고 계신 상황이겠지만 혹 도움이 될까 코멘트 남겨봅니다..!!)
그런데 지금은 이 메서드가 jdbcTemplate 메서드 호출 시에만 사용되고 있고,
TxUserService.changePassword 호출 시 커밋이 되지 않는 현상이 발생하고 있을 것 같아요.
개인적인 의견으로는 commit() 메서드가 너무 많은 일을 해야 해서 해당 현상을 해결하기가 어렵지 않나 라는 생각이 들어요.
콜백을 받아 실행하고, 반환값을 주는 것도 commit에서 해야 하나? 라는 생각도 들었구요.
그래서 아래와 같은 제안을 남겨봅니다.
- 현재 51번 라인은
startTransaction()에서, 55번 라인은finishTransaction()에서 수행하는 건 어떨까요?
public static void startTransaction(Connection connection) {
isActive.set(true);
connection.setAutoCommit(false);
}
public static void finishTransaction(Connection connection) {
isActive.set(false);
connection.setAutoCommit(true);
commitTransaction(connection);
}commit()에서 비즈니스 로직을 수행하고 결과를 반환하는 역할까지 해주어야 할까요? 중복 제거를 위해서라면, 그 대신 트랜잭션 템플릿에 대한 클래스를 별도로 만들어도 괜찮을 것 같아요.
하지만 저는 1번 방법은 생각만 해보고 2번 방법만 직접 경험해본 상태라 메리가 생각하시기에 좋은 방향으로 판단 부탁드립니당...ㅎㅎ
1번 방법을 사용할 경우 finishTransaction 메서드를 commit인 경우 / rollback인 경우로 나눠야 하고, connection 자원의 관리나 isActive 상태의 변경 사항에 대한 중복 코드가 많아지는 어려움이 있는 것 같아요.
2번 방법만 사용할 경우 finally로 자원 관리를 위한 공통 코드 처리는 쉬웠던 것 같습니다.
There was a problem hiding this comment.
아 추가로, 사실 저도 이번 미션에서는 비교적 테스트에는 집중하지 않았지만...
마안약에 시간적 여유가 되신다면 이 부분을 테스트 코드를 추가하면서 진행하면 더 수월할 것 같기도 하네요...ㅎㅎㅎㅎ (저도 해야하는데..)
테스트 자체는 다 통과해서 확인이 어려운 부분이니까요.
그게 부담이 된다면 logback 설정에서 DataSource 쪽 로그를 활성화시키거나 필요한 커스텀 로그를 찍어서 활용하는 것도요.
저는 리뷰어의 추천으로 CP를 적용하면서 logback에서 해당 로그 설정도 함께 활성화했는데,
원하는 동작대로 이루어지는지 확인하기에 조금 더 수월했던 것 같아요.
이미 잘 알고 계실 것 같지만 몇 마디 더 적어보았습니다 🥺
There was a problem hiding this comment.
도이가 제안해주신 1번 방식이 제가 코드를 제출할 때 어렴풋하게 그렸던 로직인 것 같습니다! 그런데 그러기 위해서는 DataSource를 TxUserService에서 필드로 가져야 하고, DataSource를 TxUserService에서 갖도록 하는 게 어색하다고 느껴져 고민하다 시간이 다 되어 이렇게 제대로 동작하지 않는 코드를 제출하게 되었어요 😭
말씀해주신 것처럼 1번 방식으로 코드를 작성하면서 커넥션에 대한 관리가 조금 더 정돈이 되었던 것 같습니다! 이후에 확인해보니 TxUserService에서 트랜잭션을 시작하고 종료하는 로직이 반복되어 사용되는 것을 보았습니다. 그래서 위에서 언급한 것처럼 TransactionManger를 생성해 트랜잭션 시작하고 닫아주는 책임을 넘겨주고, 해당 클래스에서 비즈니스 로직 메서드를 전달 받아 하나의 트랜잭션 내에서 비즈니스 로직이 수행될 수 있도록 리팩토링했습니다!
초반에는 TxUserService에서 트랜잭션 로직을 다시 한번 분리한다면 TxUserService에서의 역할이 사라진다고 생각했습니다. 그런데 트랜잭션 관리하기 위한 도구로 TransactionManager를 필드로 갖고 사용하는 것만으로도 적절히 역할이 분리되면서 트랜잭션에 대한 관리의 책임도 있다는 생각이 들어 이와 같이 리팩토링하게 되었습니다.
꼼꼼하게 확인하고 정성스럽게 리뷰 남겨주셔서 너무 감사해요!!
| connection.close(); | ||
| DataSourceUtils.releaseConnection(connection, dataSource); |
There was a problem hiding this comment.
P2. 현재 93번 메서드 내에서도 connection.close()를 호출하여 닫힌 자원에 대한 close를 호출하고 있어요!
There was a problem hiding this comment.
급하게 작성하면서 트랜잭션을 열고 닫는 흐름을 정확히 인지하지 못했던 것 같아요 😭
리뷰하는 데 이해가 안됐을 것 같아 죄송하네요ㅠㅠㅠ
수정해두었습니다!
| @@ -4,7 +4,7 @@ | |||
| import java.sql.SQLException; | |||
|
|
|||
| @FunctionalInterface | |||
| public interface executeQueryCallback<T> { | |||
| public interface ExecuteQueryCallback<T> { | |||
| public <T> T execute(final TransactionCallback<T> callback) { | ||
| try { | ||
| final Connection connection = TransactionSynchronizationManager.startNewTransaction(dataSource); | ||
|
|
||
| final T result = callback.execute(); | ||
|
|
||
| commitTransaction(connection); | ||
| TransactionSynchronizationManager.finishTransaction(dataSource); | ||
|
|
||
| return result; | ||
| } catch (final SQLException ex) { | ||
| throw new RuntimeException("실행 중 예외가 발생했습니다."); | ||
| } | ||
| } | ||
|
|
||
| public static void commitTransaction(final Connection connection) { | ||
| try { | ||
| connection.commit(); | ||
|
|
||
| clear(connection); | ||
| } catch (final SQLException ex) { | ||
| rollback(connection); | ||
|
|
||
| throw new DataAccessException("실행 중 예외가 발생했습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2. finally 블록을 이용해, callback.execute(); 에서 예외가 발생했을 때에도 rollback을 시켜주는 건 어떨까요?
지금은 commit을 호출했을 때의 예외에 대해서만 rollback을 시켜주는 것 같아요!
| public static Connection getResource(final DataSource dataSource) { | ||
| if (!resources.get().containsKey(dataSource)) { | ||
| final Connection connection = startNewTransaction(dataSource); | ||
| resources.get().put(dataSource, connection); | ||
| } | ||
| final Connection connection = dataSource.getConnection(); | ||
| bindResource(dataSource, connection); | ||
| return connection; | ||
| return resources.get().get(dataSource); | ||
| } |
There was a problem hiding this comment.
P3. 지난 리뷰에서 제가 놓친 부분인데요, resources.get()을 한 번만 하는 것은 어떨까요?!
| public <T> T execute( | ||
| final String sql, | ||
| final ExecuteQueryCallback<T> callBack, | ||
| final Object... objects | ||
| ) throws SQLException { | ||
| final PreparedStatement preparedStatement = createPreparedStatement(sql); | ||
| setPreparedStatement(preparedStatement, objects); | ||
|
|
||
| setPreparedStatement(preparedStatement, objects); | ||
|
|
||
| return TransactionSynchronizationManager.commit(callBack, preparedStatement, dataSource); | ||
| } catch (SQLException ex) { | ||
| TransactionSynchronizationManager.rollback(dataSource); | ||
| return callBack.execute(preparedStatement); | ||
| } | ||
|
|
||
| log.error(ex.getMessage()); | ||
| throw new CannotGetJdbcConnectionException("jdbc 연결에 실패했습니다."); | ||
| } | ||
| private PreparedStatement createPreparedStatement(final String sql) throws SQLException { | ||
| final Connection connection = TransactionSynchronizationManager.getResource(dataSource); | ||
| return connection.prepareStatement(sql); | ||
| } |
There was a problem hiding this comment.
P3. TxUserService가 아닌 AppUserService의 메서드를 호출한다면, connection이 계속 열려있지 않을까요?
안녕하세요 도이!!
4단계 미션 제출합니다!
우선 요구사항대로 서비스 레이어를 추상화해 트랜잭션 관리 로직과 비즈니스 로직을 분리해보려고 했습니다.
급하게 짜다 보니 테스트가 없어서 트랜잭션이 제대로 보장되고 있지 않은 코드가 된 것 같은데, 이 부분은 보완하도록 하겠습니다.
꼼꼼한 리뷰 많이 남겨주셔서 감사합니다!
이번 리뷰도 잘 부탁드리겠습니다!!