-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JDBC 라이브러리 구현하기 - 4단계] 허브(방대의) 미션 제출합니다. #515
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 코드보다 깔끔해지다니.. 대단합니다 허브신 🌿
남긴 리뷰가 connection realse 해주는 부분인데..
이 부분만 한번 확인 부탁드립니드아..!
다음 미션도 화이팅!👍
try (final PreparedStatement preparedStatement = statementCreator.create(connection, sql, parameters)) { | ||
return preparedStatementCallback.execute(preparedStatement); | ||
} catch (final SQLException e) { | ||
log.error(e.getMessage(), e); | ||
throw new DataAccessException(e); | ||
} finally { | ||
connectionManager.release(connection); | ||
DataSourceUtils.releaseConnection(connection, dataSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
허브신🌿 여기서 realseConnection
을 실행해준 이유 알 수 있을까요?
여기서 수행하게 되면 UserDao
와 UserHistoryDao
에서 한쪽이 커낵션을 종료할 수 도 있기 때문에
ConnectionHolder
로 트랜잭션이 진행중인지 확인하도록 되어있는거 같은데
TransactionTemplate
에서 수행하면 AppUserService
의 메소드가 끝난 후에 진행하니까
현재 ConnectionHolder
에서 트랜잭션이 진행중인지 확인 안해도 되지 않을까요??
혹시 다른 의도가 있었던 걸 까요 허브신 🌿?!
(아니면 메모리 누수?!)
(아니면 나중에 트랜잭션 전파를 활용하기 위해서 빼신 것일까요?!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransactionTemplate을 사용하지 않는 상황에서는 자원 해제를 해줄 필요가 있다고 생각하여 releaseConnection 메서드를 호출하도록 해두었습니당!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗.. Transaction만 생각하다보니 Transaction을 사용하지 않는 경우를 생각 못했네요.. 🥹
private TransactionSynchronizationManager() { | ||
} | ||
|
||
private static Map<DataSource, ConnectionHolder> resource() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세심함 .. 👍
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment; | ||
|
||
@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) | ||
class Stage0Test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
역시 허브신🌿 aop 학습테스트까지 진행하시다니..
안녕하세요 호이 😄
전 단계에서 코드 꼼꼼하게 봐주셔서 너무 감사합니다 🙇
변경사항
기존의 3단계에서 사용했던 코드를 제거하고, 4단계에서 제공하는 코드를 사용하도록 수정했습니다.
그 과정에서 TransactionTemplate이 직접적으로 Connection을 받지 않고, TransactionManager라는 클래스를 통해 connection에 대한 설정을 하도록 변경했습니다!
코드를 최대한 간단히 변경하기 위해 TransactionTemplate에서는 표준 함수형 인터페이스를 사용하도록 변경했습니다.
기존의 ThreadLocal을 이용해 Connection을 가지고 있는 부분을 제공된 클래스인 TransactionSynchronizationManager로 이동시키고 단순히 Connection이 아닌 Connection과 transactionActive 플래그를 들고 있는 ConnectionHolder 클래스를 가지도록 변경했습니다.
간단히 정리하자면 다음과 같습니다.
TransactionTemplate: 동일
ConnectionManager -> TransactionManager
ConnectionHolder -> 기존 동작을 TransactionSynchronizationManager 이동시키고, Connection과 transactionActive 가지는 클래스로 변경
4단계도 잘 부탁드립니다 😃