-
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단계] 블랙캣(송우석) 미션 제출합니다🚀 #580
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.
안녕하세요 블랙캣!
어제 슬랙으로 말씀해주셨던 것처럼 이번에는 프록시 패턴 적용해주신 부분 확인했습니다!
3단계에서 이미 잘 구현해주셔서 큰 수정사항 없이도 4단계를 잘 구현하실 수 있었던 것 같습니다👍그래서 리뷰할 내용이 거의 없었어요..!
슬랙에서 말씀해주신 것처럼 원하는 방향으로 리팩토링 진행하시고 편하게 다시 요청 주세요. 고생 많으셨습니다!
import org.junit.jupiter.api.Test; | ||
import org.springframework.dao.DataAccessException; | ||
import org.springframework.jdbc.core.JdbcTemplate; | ||
import org.springframework.jdbc.support.TransactionTemplate; | ||
|
||
@Disabled | ||
class UserServiceTest { | ||
|
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.
꼼꼼한 리뷰 감사합니다!
) { | ||
return callback.callback(preparedStatement); | ||
} catch (final Exception e) { | ||
log.error(e.getMessage(), e); | ||
throw new DataAccessException(e); | ||
} | ||
} | ||
|
||
public PreparedStatement createPrepareStatement( |
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.
지난번에 고민하셨던 부분이었던 것 같은데 jdbcTemplate으로 옮겨주셨네요!
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.
저번에 밀리의 리뷰를 들어보고 고민해보니 아무래도 jdbcTemplate
가 더 맞는거 같아서 복구 시켜놓았습니다!
@@ -41,6 +45,7 @@ void findByAccount() { | |||
final var user = userDao.findByAccount(account); | |||
|
|||
assertThat(user.getAccount()).isEqualTo(account); | |||
System.out.println(user.getId()); |
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.
안녕하세요 블랙캣!
테코톡 준비랑 병행하시느라 고생 많으셨습니다!!
1단계부터 이미 완성도 높은 코드였는데 제안해주셨던 방식대로 잘 리팩토링 해주셔서 많이 배웠습니다 감사해요!
이번에 반영해주신 부분에 대해 질문이 있어서 코멘트 두 개 남겼는데, 여유있을 때 확인 부탁드리겠습니다!
이미 잘 구현해주신 코드라 approve하겠습니다.
내일 테코톡도 화이팅!!
|
||
private static void clear() { | ||
connections.get().clear(); | ||
TransactionSynchronizationManager.rollback(); |
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.
한 가지 궁금했던 점은 해당 메서드에서는 TransactionalSynchronizationManager를 호출하는 용도로만 사용하고 있는데,
추상화를 위해 중간 단계를 두신건지 궁금합니다!
public static Connection getResource(final DataSource dataSource) { | ||
if (isTransactionActive.get()) { | ||
final Connection connection = guaranteeConnection(dataSource); | ||
beginTransaction(connection); |
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.
guaranteeConnection()
에서 datasource에 매핑되는 커넥션이 없으면 새로 생성해 반환하고, 있으면 기존에 DataSource에 매핑되어 있던 커넥션을 반환해주는 것이라고 이해했습니다.
그러면 기존에 이미 매핑되어 있던 커넥션은 autoCommit이 false으로 설정되어 있다고 이해했습니다.
새로운 커넥션을 가져올 때 바로 beginTransaction()
을 해주지 않고 이후에 해주신 이유가 궁금합니다!
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.
머지를 시켰는데 궁금한 점이 생겨서 또 다른 리뷰를 남기게 되었습니다...
시간 될 때 코멘트 부탁드려요!!
public Connection getConnection() { | ||
return connection; | ||
} | ||
|
||
@Override | ||
public void close() throws Exception { | ||
if (!isTransactionActive) { |
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을 close하기 전 autoCommit을 true로 변경해주어야 합니다!
커넥션 풀에 저장된 커넥션을 돌려서 사용하기 때문에 autoCommit이 false인 상태로 close를 하면 나중에 다른 스레드에 할당될 커넥션의 autoCommit이 false인 상태로 커넥션을 할당 받기 때문이라고 합니다!
@@ -16,16 +12,6 @@ private ConnectionHolder(final Connection connection, final boolean isTransactio | |||
this.isTransactionActive = isTransactionActive; |
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.
그리고 지난번에 DM으로 말씀해주셨던 부분인데 궁금한 점이 생겨 질문 드려요!
DataSourceUtils 구현 부 때문에 자바에서 제공하는 try-with-resources 를 활용하지 못한다는 느낌이 들었어요!
저는 블랙캣이 해당 부분을 해소하기 위해 ConnectionHolder에서 AutoCloseable을 구현하셨다고 생각했습니다.
해당 부분을 적용해주신 뒤에 질문 주셨던 내용이었는지 기억이 안 나네요..
저는 ConnectionHolder에서 AutoCloseable을 구현하면서 close()를 재정의해주면서 try-with-resources에 사용할 수 있게 되었다고 생각했는데 제가 이해한 게 맞을까요??
안녕하세요 메리! 👋
이번 step4 미션 제출합니다!
cglib 를 이용해서
@Transaction
를 구현해보고 싶었지만 시간이 너무 부족하네요 😭나중에 한번 해봐야겠습니다..
슬랙에서 말씀 드렸던 것 처럼 미션에서 제공하는 뼈대코드는 사용하지 않았습니다.
그래서 현재는
TransactionManager
에서connection
생성, 트랜잭션 시작/커밋/롤백을 담당하고 있습니다.이부분은 메리랑 이야기 해보면서 리팩터링 해보겠습니다 아마 아래와 같은 구조로 하지 않을까? 싶네용 🤔
이번 step4 도 잘부탁드립니다!