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

[4단계 - JDBC 라이브러리 구현하기] 도비(김도엽) 미션 제출합니다. #863

Open
wants to merge 5 commits into
base: dobby-kim
Choose a base branch
from

Conversation

Dobby-Kim
Copy link

안녕하세요 뽀오오오온드

벌써 마지막 리뷰 요청이군요!

이번 리뷰에서는 마지막 리뷰인만큼 여유를 가지고 여러 얘기 나눠보면 좋을 것 같습니다 :>

리뷰 잘부탁드린다구요😘😘

@Dobby-Kim Dobby-Kim self-assigned this Oct 14, 2024
Copy link

@tackyu tackyu left a comment

Choose a reason for hiding this comment

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

안녕하세요 도비
4단계까지 고생 많으셨습니다!👍👍
몇가지 코멘트 남겼는데, 확인 부탁 드릴게요
다음에 학습테스트도 같이 제출해주시면 좋을 것 같습니다!

Comment on lines +45 to +46
TransactionSynchronizationManager.unbindResource(dataSource);
}
Copy link

Choose a reason for hiding this comment

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

unbindResource(dataSource)
releaseConnection 안에서 수행되는 것은 어떻게 생각하세요??

Copy link
Author

Choose a reason for hiding this comment

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

unbindedResource는 단순히 바인딩을 끊는 용도로도 사용되어야된다고 생각합니다. 같은 Transaction을 사용하다가도, 사용하지 않을 수도 있어야하니까요 :>

Comment on lines +39 to +40
try {
conn.rollback();
Copy link

Choose a reason for hiding this comment

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

rollback()을 명시하게끔 수정하신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

Transaction 안에서의 sql 쿼리가 실패하면 rollback 해야하지 않을까요?

Copy link

Choose a reason for hiding this comment

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

try (Connection conn = DataSourceUtils.getConnection(dataSource)) {
         conn.setAutoCommit(false);
         User user = updateUserPassword(conn, id, newPassword);
         userHistoryDao.log(conn, new UserHistory(user, createBy));
         conn.commit();
     } catch (SQLException e) {
         throw new DataAccessException("Failed to change password", e);
     }

이전 단계에서 try-catch로 처리 하셔서 궁금했습니다!

Comment on lines 12 to 13
void changePassword(final long id, final String newPassword, final String createdBy) throws SQLException;
}
Copy link

Choose a reason for hiding this comment

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

예외를 직접 처리해주셔서 throws가 없어도 될 것 같아요!

Comment on lines +89 to +91
public DataSource getDataSource() {
return dataSource;
}
Copy link

Choose a reason for hiding this comment

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

dataSource를 반환하게끔 설계하신 이유가 궁금합니다
update와 같은 메서드에서 직접 커넥션을 가져와, 메서드 오버로딩을 없애는 것에 대해 도비의 의견이 궁금하네요!

Copy link
Author

@Dobby-Kim Dobby-Kim Oct 20, 2024

Choose a reason for hiding this comment

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

해당 jdbcTemplate에서 datasource를 가져와야 Util에서 map으로 존재하는 같은 connection을 찾아 들고 올 수 있게 했어야했습니다!

여러 datasource가 존재할 때가 있을 수 있으니까요!

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