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 3,4단계] 키아라 미션 제출합니다 #554

Merged
merged 18 commits into from
Oct 15, 2023

Conversation

kiarakim
Copy link

@kiarakim kiarakim commented Oct 9, 2023

🐸케로케로케로케🐸 안녕하세요 🦁키아라키아라키아라키ㅇ🦁입니다
이번엔 3,4 단계를 한번에 제출하게됐어요.허허

3단계에서 트랜잭션 경계 설정을 하고 넘어갔다 생각하고 4단계를 진행하던 중 한 고민에 빠져버렸읍니더
UserService엔 changePassword() 말고도 findById() 가 있죠.
changePassword()는 트랜잭션을 시작하고 findById()로 id를 갖고 온 후에 log까지 찍고 트랜잭션을 종료합니다. 하나의 트랜잭션에서 진행하기 위함이죠. 그런데 findById()도 마찬가지로 어떤 이유에서 이 메서드 하나만 사용할 때 트랜잭션을 적용하고 싶다면 findById()에서도 마지막에 트랜잭션을 종료(commit, rollback)해야하죠. 그럼 changePassword()에서 호출하고 돌아갈 때 트랜잭션이 종료되버리면 안되는데 이 이걸 어떻게 해결해야 할지 도통 모르겠습니다.
그래서 살짝 이게 맞나 싶은 방법이긴 한데 커넥션이 있는지 확인한 후(지금 생각해보면 열려있는지도 확인했어야 했는데) 있으면 거기에 참여하고 없으면 새 커넥션을 받아 트랜잭션 시작하도록했습니다..

제가 요구사항을 잘 만족했는지 좔 모르겠네욥ㅎㅅㅎㅠㅠ 이번에도 리뷰 잘 부탁드립니다!

@jyeost jyeost self-requested a review October 11, 2023 08:55
Copy link

@jyeost jyeost left a comment

Choose a reason for hiding this comment

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

안녕하세요 키아라 !
개인 사정으로 리뷰가 늦어져서 죄송함돠...

3-4단계 잘 해주셨는데 좀 더 수정됐으면 좋을 부분들이 보여 리뷰 남겼습니다 !

혹시 납득 되지 않거나 이해 되지 않는 부분이 있으면 답글 남겨주시거나 디엠 주십셔!!!!

@@ -11,13 +15,40 @@ public abstract class TransactionSynchronizationManager {
private TransactionSynchronizationManager() {}

public static Connection getResource(DataSource key) {
return null;
Map<DataSource, Connection> map = getMap();
Copy link

Choose a reason for hiding this comment

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

13 line에 ThreadLocal의 withInitial static함수를 사용하여 초기화 해준다면
map이 들어있는지 매번 확인하는 로직을 줄일 수 있을 것 같아요 !

Copy link
Author

Choose a reason for hiding this comment

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

그런 함수가 있었군요 감사합니다!

@@ -29,6 +29,7 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd

public static void releaseConnection(Connection connection, DataSource dataSource) {
try {
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.

connection은 getAutoCommit 메서드를 사용하여 해당 커넥션이 트랜잭션을 위한 커넥션인지 확인하고 종료시켜주면 좀 더 유연하게 사용할 수 있을것 같아염

(커넥션을 종료하는 메서드가 현재 JdbcTemplate과 TxService 에 존재하니까 어떨때 어떤 커넥션을 꺼주는 지 생각해 보면 댈듯요 ㅎㅅㅎ)

Copy link
Author

Choose a reason for hiding this comment

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

connection.getAutoCommit()이 false일 때 닫아주도록 변경했는데 혹시 의도하신 부분이 맞나욤?

String sql = "insert into users (account, password, email) values (?, ?, ?)";
log.info("[LOG] insert user into users");
jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail());
jdbcTemplate.update(connection, sql, user.getAccount(), user.getPassword(), user.getEmail());
Copy link

Choose a reason for hiding this comment

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

jdbcTemplate도 DataSource를 가지고 있으니, dao에서 반복적으로 전달하기 보단

getConnection과정도 JdbcTemplate에서 해주는 것은 어떤가여??

dao에서 connection을 관리하는 것이 책임 분리가 되었다고 느껴지지 않아서영,,,

Copy link
Author

Choose a reason for hiding this comment

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

기억속엔 connection 파라미터를 없앤걸로 알고 있는데 이렇게나 많이 남아있었다니.. 발견해주셔서 감사합니다

} catch (SQLException ignored) {}
}
log.info("[LOG] insert user into user_history");
jdbcTemplate.update(connection, sql, userHistory.getUserId(), userHistory.getAccount(), userHistory.getPassword(), userHistory.getEmail(), userHistory.getCreatedAt(), userHistory.getCreateBy());
Copy link

Choose a reason for hiding this comment

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

파라미터가 길어진다면 배열로 전달해도 좋을 것 가타여!

Copy link
Author

Choose a reason for hiding this comment

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

앗 넵!

map.put(key, connection);
}
return connection;
} catch (SQLException e) {
Copy link

Choose a reason for hiding this comment

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

DataSourceUtils 에서 해당 메서드를 사용하고 있는데 Connection이 null이라면 새로운 커넥션을 만들어서 return하고 있어요

여기서도 connection이 없다면 그냥 null을 return해주는 것은 어떤가여??

Copy link
Author

Choose a reason for hiding this comment

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

중복된 커넥션 생성 로직을 제거하니 위 클래스가 한결 깔끔해졌네요!👍

connection = DataSourceUtils.getConnection(dataSource);
connection.setAutoCommit(false);
User user = userService.findById(id);
connection.commit();
Copy link

Choose a reason for hiding this comment

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

commit과 rollback에 대해 공부해보면 좋을 것 같아요 !

transaction이 필요하지 않은 로직은 JdbcTemplate에서 커넥션을 관리하고 있으므로
서비에스에서 관리해 줄 필요 없을 것 같아요 ㅎㅅㅎ

그리고 conncetion을 close하는 메서드는 반드시 finally 문 안에 있어야해요!
그래야 예외가 발생하여도 conncetion이 해제 되지 않아서 resource가 낭비되는 걸 막을 수 있습니다 !

} catch (SQLException e) {
connection.rollback();
throw new SQLTransactionRollbackException(e.getMessage());
} finally {
Copy link

Choose a reason for hiding this comment

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

transaction을 사용 완료한 connection의 autocommit을 다시 켜준다면

DataSourceUtil에서 해당 커넥션이 여러 작업을 수행하고 있는지 알 수 있을 것 같아요 ㅎㅅㅎ

그럼 realseConncetion에서 종료할 작업을 잘 분리할 수 있을 것 같아요

@Override
public void changePassword(long id, String newPassword, String createBy) throws SQLException {
Connection connection = DataSourceUtils.getConnection(dataSource);
connection.setAutoCommit(false);
Copy link

Choose a reason for hiding this comment

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

setAutoCommit도 SQL Exception을 발생시킬 수 있는데 try문에 안 넣어도 될까요??

@Override
public void insert(User user) throws SQLException {
Connection connection = null;
try {
Copy link

Choose a reason for hiding this comment

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

try - catch 와 커넥션을 만들고 커밋/롤백하는 작업이 반복되고 있는데...

이 부분을 분리하여 다른 메서드로 정의하고

이 클래스의 메인 로직인 userService의 메서드를 파라미터로 넘기면 중복을 제거할 수 잇을 것 같아요!

@kiarakim
Copy link
Author

케로🐸 저를 끝까지 포기하지 않아주셔서 감사합니다💚
확실히 중복된 로직을 제거하니 코드가 훨씬 깔끔해진 것 같아요.
특히 TxUserService와 TransactionSynchronizationManager에서 많은 부분들을 짚어주셔서 도움이 많이 됐습니다!

제 리뷰이는 템플릿 콜백을 사용하셨더라구요. 해당 부분을 잘 몰라서 이번 기회에 공부해봤는데 TxUserService에서 connection을 가져와 transaction을 시작하고 종료하는 중복 로직에서 사용하면 좋을 것 같아 적용해봤습니다.

덕분에 미션에서 얻어간게 많네요👍 저희 자동차 경주 첫 페어로 만나 거의 마지막 미션도 함께하게 됐는데 케로... 대단한 사람이 되셨어요 넘 멋진 거🥹
정성 가득한 코드 리뷰 감사합니다~ 다음 미션도 화이팅!

@jyeost jyeost self-requested a review October 15, 2023 09:26
@FunctionalInterface
public interface TransactionExecutor<T> {

T execute();
Copy link

Choose a reason for hiding this comment

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

return이 void인 인터페이스를 만든다면 서비스에 return null이 반복되는 것도 줄여줄 수 있을 것 같아용 :)

}

private <T> List<T> query(String sql, RowMapper<T> rowMapper, PreparedStatementSetter pss) {
Connection connection = DataSourceUtils.getConnection(dataSource);
Copy link

@jyeost jyeost Oct 15, 2023

Choose a reason for hiding this comment

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

try -resource-catch 안에 Connection이 들어있지 않아서 끝나고 connection이 close되지 않으면....
리소스가 낭비됩니다!!!! 만약 커넥션 풀을 사용중이라면 회수 되지 않은 커넥션으로 인해 어플리케이션이 멈출수도 있지 않을까요...??

또 JdbcTemplate에서도 사용이 완료된 커넥션은 finally문을 사용하여 releaseConnection()해주는 것이 좋겠어요 ㅎㅅㅎ

@@ -29,7 +29,10 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd

public static void releaseConnection(Connection connection, DataSource dataSource) {
try {
connection.close();
if(!connection.getAutoCommit()){
Copy link

Choose a reason for hiding this comment

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

제가 이부분을 언급했던건
Connction을 사용하고 반납하는 곳이

JdbcTemplate : transaction 사용x, autoCommit -> true
TransactionTemplate : transaction 사용중이면 autoCommit -> false
transaction 사용 완료 후 autoCommit -> true

이기 때문에 connection의 commit을 확인해서 언바인딩을 해줬으면 했던 것이였어여 !! 근데 식이 반대가 된 거 같아요ㅜㅜ

Copy link

@jyeost jyeost left a comment

Choose a reason for hiding this comment

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

키아라 리뷰 반영 너무 잘해주셨어요 !!!
리뷰를 최대한 열심히 쓰려고 노력했는데,, 다 제대로 이해하고 반영해주신 것 같아서 저도 뿌듯합니다
바쁘다고 하셨는데,,, 새로운 미션 하랴 팀플하랴 Jdbc 끌고가랴 수고 많으셨습니다...
저도 키아라의 꾸준했던(?) 블로그를 보며 힘냈던 날들이 생각남니다... 아직 끝이 아니기에 !!!!
다시 힘내서 함께 열심히 달려갑시다...@@!!!

@jyeost jyeost merged commit a699026 into woowacourse:kiarakim Oct 15, 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