Skip to content
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단계] 우르(김현우) 미션 제출합니다. #550

Merged
merged 6 commits into from
Oct 10, 2023

Conversation

java-saeng
Copy link

안녕하세요 히이로 !!

저번 좋은 리뷰 잘 받았습니다!

메서드를 템플릿 콜백 패턴으로 중복을 줄여보려고 했는데,,개인적으로 제가 코드를 이해하는게 더 어려워보였습니다,,
중복 코드를 제거하는건 좋지만,, 저는 가독성이 더 높아야한다고 생각해서요!

트랜잭션 매니저에서 실제 커넥션 객체가 어떤 방식으로 관리됐는지 물어보는 해당 리뷰는 제가 공부해보고 답변달도록 해볼게요..

Copy link

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 우르!
마지막까지 미션 해주시느라 고생 많으셨습니다! 저번 코드가 워낙 깔끔하게 떨어지도록 구현 잘 해주셨어서 이번에도 비슷하게 진행된 것 같아 리뷰드릴 부분이 많이 없었네요!

수정 요청드릴 부분은 사소한 부분이고, 그 외는 학습해보시면 좋을 것 같은 내용들 위주로 리뷰드려봤습니다! 반영해주시면 바로 Merge 하도록 할게요! Jdbc 미션동안 우르 코드를 리뷰하며 많이 배웠습니다. 고생하셨어요~ 👍

Comment on lines +22 to +30
private static Connection createConnection(final DataSource dataSource) {
try {
final Connection connection = dataSource.getConnection();
TransactionSynchronizationManager.bindResource(dataSource, connection);
return connection;
} catch (SQLException ex) {
throw new CannotGetJdbcConnectionException("Failed to obtain JDBC Connection", ex);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

세심하게 메서드 분리 잘 해주셨네요 😄

try {
connection.setAutoCommit(false);
} catch (SQLException e) {
throw new RuntimeException(e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소하긴 하지만 여기도 DataAccessException으로 함께 바꿔주시면 좋을 것 같습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

얕은 지식이지만 첨언드려보자면... 스프링에서는 spring-jdbc와 spring-tx 라이브러리가 구분되어 존재하고 있더라구요! 그리고 DataAccessException은 spring-tx 라이브러리에 속하는 예외 클래스입니다. 같은 SQLException이더라도 예외 전환될 때 spring-jdbc, spring-tx에 걸쳐 구현되어 있는 예외 클래스들로 전환이 되는데 지금 같은 경우는 트랜잭션 내에서 발생하는 예외 상황이다 보니 spring-tx에 속하는 DataAccessException을 상속한 어떤 예외 클래스로 전환이 될 것 같네요 ㅎㅎ

이미 알고 계실지도 모르지만 조금이나마 도움이 되셨으면 하는 마음으로 적어보았습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 spring-jdbc 라이브러리의 support 패키지 내 SQLErrorCodeSQLExceptionTranslator클래스와 sql-error-codes.xml을 참고해보시면 학습에 도움이 되실 것 같습니다. 😃

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와우,, spring-tx 에는 트랜잭션 관련 클래스들이 다 있네요,, 몰랐습니다 !! 좋은 정보 감사합니다 :)

트랜잭션 내에서 발생한 예외라 spring-tx에 있는 예외로 반환될 것 같네요

.put(dataSource, connection);
}

private static void setAutoCommit(final Connection connection) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 메서드의 책임은 autoCommit을 false로 세팅해주는 것인데 setAutoCommit 이라는 네이밍만으로는 조금 모호한 것 같아요... 좀 더 구체적인 네이밍으로 바꿔보시는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오,, 그렇네요 감사합니다 !

Comment on lines +27 to +40
@Override
public void changePassword(final long id, final String newPassword, final String createBy) {
TransactionSynchronizationManager.start();

try {
userService.changePassword(id, newPassword, createBy);
TransactionSynchronizationManager.commit(dataSource);
} catch (Exception e) {
TransactionSynchronizationManager.rollback(dataSource);
throw e;
} finally {
TransactionSynchronizationManager.release(dataSource);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라이브러리를 사용하는 입장에서는 일일이 커넥션을 관리할 필요 없이 TransactionSynchronizationManager를 통해 편하게 트랜잭션 관리를 할 수 있게 되었네요! 👍

}

public static void release(final DataSource dataSource) {
final Connection connection = resources.get().get(dataSource);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 부분에서 null 처리를 굉장히 꼼꼼하게 해주셨어요! 여기에도 null 처리 로직을 추가해주시면 어떨까요? 만약 connection이 없는 상태로 뒤 로직들이 수행되면 NPE가 발생할 것 같습니다!

@java-saeng
Copy link
Author

안녕하세요 히이로 !! 리뷰 잘 받았습니다 덕분에 spring-tx 패키지에 따로 있는지 처음 알았습니다,,

해당 리뷰 에 대해서 공부를 조금 해보았는데 이게 맞는지 모르겠어요,, 틀린 부분이 있다면 말씀해주세요!!

JpaTxManager, JdbcTxManager 들이 트랜잭션을 시작한다면 TxSyncManager에서 Resource들을 가져오더라구요.

TxSyncManager에서는 ThreadLocal 로 Map<Object, Object>로 가지고 있구여.

그래서 TxSyncManager에서 이러한 Resource들을 관리하지 않나 싶습니다!!

Copy link

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

햐... 지난번에 질문드린 부분에 대해서 이렇게 정리해주실 줄은 몰랐네요... 덕분에 실제 트랜잭션 매니저에서도 ThreadLocal로 자원들을 관리한다는 걸 직접 파악할 수 있었습니다. 👍

우르와 이번 미션 함께 진행하면서 정말 많은 것들 배워갑니다. 고생 많으셨고 다음 미션도 화이팅입니다~!!!

@MoonJeWoong MoonJeWoong merged commit af402af into woowacourse:java-saeng Oct 10, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants