-
Notifications
You must be signed in to change notification settings - Fork 378
[3단계 - Transaction 적용하기] 배키(백은희) 미션 제출합니다. #811
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
Conversation
seokmyungham
left a comment
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 connection = null; | ||
| try { | ||
| connection = dataSource.getConnection(); | ||
| connection.setAutoCommit(false); | ||
|
|
||
| userDao.update(user, connection); | ||
| userHistoryDao.log(new UserHistory(user, createBy), connection); | ||
|
|
||
| connection.commit(); | ||
| } catch (SQLException e) { | ||
| rollback(connection); | ||
| throw new DataAccessException(e); | ||
| } | ||
| } |
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.
사용한 커넥션을 정리하지 않아도 괜찮을까요?
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.
현재 비밀번호 변경 로직은 유저 조회와 비밀번호 변경 작업이 각각 다른 커넥션을 사용하고 있는 것 같아요.
트랜잭션 경계는 어떻게 설정해야 바람직할까요? 현재는 어떤 문제가 발생하게 될까요?
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.
SQLException를 catch하고 있는데 혹시 비즈니스 로직 수행중에 NullPointerException나 IllegalArgumentException이 발생하면 어떻게 되는지 궁금해요
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.
사용한 커넥션을 정리하지 않아도 괜찮을까요?
앗 그렇네요.. 커넥션을 닫도록 수정했습니다 :)
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.
현재 비밀번호 변경 로직은 유저 조회와 비밀번호 변경 작업이 각각 다른 커넥션을 사용하고 있는 것 같아요.
트랜잭션 경계는 어떻게 설정해야 바람직할까요? 현재는 어떤 문제가 발생하게 될까요?
지금 당장은 제일 처음에 조회하는 로직이기 때문에 문제가 없다고 생각했는데, 제가 잘못 생각했네요😅
조회 로직과 업데이트 로직은 서로 다른 커넥션을 사용하기 때문에, 조회 시점과 업데이트 시점 사이에 다른 트랜잭션에 의해 데이터가 변경되는 문제가 생길 것 같아요.
추가로, 조회와 수정이 다른 커넥션에서 이루어지기 때문에 조회된 사용자 객체가 다른 트랜잭션에 의해 업데이트되었다면 조회된 값은 최신 상태를 반영하지 못합니다. 이렇게 같은 비즈니스 로직인데 커넥션을 달리 가져가면 관리가 매우 어려워질 것 같아요.
때문에 기본적으로 하나의 트랜잭션은 하나의 커넥션으로 관리되는 것이 좋습니다. 하나의 커넥션만을 사용하도록 수정하겠습니다😊
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.
SQLException를 catch하고 있는데 혹시 비즈니스 로직 수행중에 NullPointerException나 IllegalArgumentException이 발생하면 어떻게 되는지 궁금해요.
현재 코드에서 SQLException만 catch하고 있기 때문에, RuntimeException이 발생할 경우에는 별도로 처리되지 않습니다. 이러한 예외가 발생하면 트랜잭션이 롤백되지 않고 커넥션도 닫히지 않습니다! 이를 해결하기 위해서 Exception을 catch 하도록 수정했습니다~!
|
|
||
| public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... args) { | ||
| return execute(sql, pstmt -> mapResults(rowMapper, pstmt.executeQuery()), args); | ||
| return execute(sql, pstmt -> mapResults(rowMapper, pstmt.executeQuery()), getConnection(), args); |
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.
커넥션을 이 시점에 획득하는 이유가 있을까요? 🤔
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.
execute함수가 connection을 매개변수로 받는 함수밖에 없어서 그렇게 했습니다.. 그런데 해당 함수는 JdbcTemplate 내부의 필드로 존재하는 datasource에서 꺼내온 connection을 사용하기 때문에 꼭 connection을 넘길 필요가 없을 것 같습니다.
execute 함수를 오버로딩해서 수정했습니다~!!~!
|
|
||
| public int update(String sql, Object... args) { | ||
| return execute(sql, PreparedStatement::executeUpdate, args); | ||
| return execute(sql, PreparedStatement::executeUpdate, getConnection(), args); |
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.
이 부분도 마찬가지입니다!
seokmyungham
left a comment
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.
배키 고생하셨습니다!!
더 다양한 얘기는 4단계 진행하고 나서 하면 될 것 같아요~
머지하겠습니다 👍
안녕하세요. 재즈~!
3단계로 다시 돌아왔습니다😎
미션 마감일이 좀 빠듯한 것 같아서 일단 구현에 초점을 맞춰 진행했습니다.
현재 동일한 트랜잭션에서 동일한 connection을 매개변수로 넘기는 방식인데, 기존의 모든 메서드에 변경이 일어난다는 점이 아쉽네요🥲
아마, 4단계 때 요구사항에 맞춰서 리팩토링을 진행하지 않을까 싶어요!
잘 부탁드립니다 🙇♀️