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단계] 주노(최준호) 미션 제출합니다. #532

Merged
merged 7 commits into from
Oct 11, 2023

Conversation

Choi-JJunho
Copy link
Member

땡칠 안녕하세요!
미션을 아까 10시쯤에 마무리지었는데 자기전에 PR 제출을 안했다는게 생각나써 늦은 새벽에 PR을 조심스래 올려봅니다...

마지막 미션도 잘부탁드립니다 🙏

Copy link

@0chil 0chil left a comment

Choose a reason for hiding this comment

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

안녕하세요 주노 ✋🏻 주말 잘 보내셨나요!
날씨가 오락가락~ 감기 조심하세요~

지난 커멘트에 대한 답변

Unchecked Exception 관련

좋습니다~ 저도 '로직을 중단하고 발생한 예외를 처리한다' 정도면 충분할 것 같아 Unchecked Exception으로 변환했습니다.
다만 비즈니스 로직 실행 중 발생한 Unchecked Exception도 롤백이나 Close() 해야한다는 점을 누락할까봐 걱정되더라구요.
이 문제는 중요도에 따라서 실행 인터페이스(Runnable, Supplier)에 throws Exception를 추가해 볼 수 있을 것 같습니다.


미션 고생하셨습니다!
잘 구현해주셨는데, 테스트를 깜빡하셨나봐요 ㅎㅎ
보완해야 할 부분과 질문 커멘트로 남겼으니 확인해주세요!

Comment on lines 13 to 19
public static Connection getResource(DataSource key) {
return null;
return resources.get().getOrDefault(key, null);
}

public static void bindResource(DataSource key, Connection value) {
resources.get().put(key, value);
}
Copy link

Choose a reason for hiding this comment

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

주노 테스트 깜빡하셨군요! ㅋㅎㅎ 😂

여기에서 NPE가 발생하고 있는데요, ThreadLocal의 사용법을 한 번 알아봅시다

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 이런 실수를 😂😂
null 검증을 추가했습니다~!

Comment on lines 52 to 53
final var appUserService = new AppUserService(userDao, userHistoryDao);
final var userService = new TxUserService(appUserService, new TransactionExecutor(transactionManager));
Copy link

Choose a reason for hiding this comment

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

이 테스트를 돌려보시면 예외가 발생할 거에요!
JdbcTemplate:51의 try-with-resources 에 의해 이미 Connection 객체가 닫혔기 때문인데요,
어떻게 해결하면 좋을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

JdbcTemplate의 Connection을 DatasourceUtils를 이용하여 가져오도록 수정했습니다. 이 과정에서 try-with-resource의 대상에서 제외시켰습니다.

추가로 Transaction 수행 과정에서 TransactionManager가 Connection을 필드로 가짐으로써 close된 Connection이 계속 남아있던 부분을 확인했습니다.
Transaction begin 시 Connection이 close되어있다면 connection을 새롭게 갱신하는 로직을 추가했습니다.

Copy link

Choose a reason for hiding this comment

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

JdbcTemplate의 Connection을 DatasourceUtils를 이용하여 가져오도록 수정했습니다. 이 과정에서 try-with-resource의 대상에서 제외시켰습니다.

확인했습니다. 이제 DataSourceUtils 가 커넥션을 열고 닫는 책임을 가지겠네요.

추가로 Transaction 수행 과정에서 TransactionManager가 Connection을 필드로 가짐으로써 close된 Connection이 계속 남아있던 부분을 확인했습니다.
Transaction begin 시 Connection이 close되어있다면 connection을 새롭게 갱신하는 로직을 추가했습니다.

고생하셨습니다~! 이제 닫힌 커넥션을 갱신할 수 있게 되었네요!

하지만 이 방법은 임시방편이라는 생각이 들어요.
TransactionManager의 인스턴스를 정말 하나만 만들어도 될까요?
저는 주노 코드에서 TransactionManager 객체와 '논리적 트랜잭션' 이라고 부르는 그것이 생명 주기를 같이 한다고 느껴집니다.
그래서 현재 상태로는 여러 쓰레드가 하나의 논리적 트랜잭션을 사용하는 꼴이 되므로 멀티쓰레드 환경을 지원하지 못할 것이라는 의심이 들어요.

멀티 쓰레드 환경으로 가면,
여러 쓰레드가 Service 객체 -> TransactionExecutor 객체 -> TransactionManager 객체를 공유해 사용할 거에요.
( -> 는 객체 참조입니다!)

정말 그렇게 되어도 괜찮을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

와 멀티스레드 환경 생각도 못하고있었네요...!!!
날카로운 지적 감사합니다 🙇‍♂️🙇‍♂️🙇‍♂️🙇‍♂️
execute 시 마다 별개의 TxManager를 생성하고 동작하도록 변경했습니다.

Comment on lines +38 to +44
public void rollback() {
try {
connection.rollback();
this.close();
} catch (SQLException e) {
throw new DataAccessException(e);
}
Copy link

@0chil 0chil Oct 9, 2023

Choose a reason for hiding this comment

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

커밋할 때, 그리고 예외로 인한 롤백 시에도 트랜잭션 종료 및 커넥션 제거를 잘 해주고 있네요👍

하나의 쓰레드에서 트랜잭션을 여러 번 실행했을 때,
커넥션이 잘 제거되지 않은 경우 재활용되어 기존 트랜잭션에서 미처 롤백되지 않은 사항이 반영되는 불상사가 생길 수 있습니다.

하지만 주노 코드는 커넥션까지 잘 제거해서 이런 사고를 잘 막을 수 있겠네요!
저는 이 부분 빠뜨렸는데, 잘 배워갑니다!

Comment on lines 52 to 53
final var appUserService = new AppUserService(userDao, userHistoryDao);
final var userService = new TxUserService(appUserService, new TransactionExecutor(transactionManager));
Copy link

Choose a reason for hiding this comment

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

저는 TxUserService로 AppUserService를 감싸서 사용하는게 엄청 부자연스럽게 느껴졌어요.
건드려면 일이 커질 것 같아 우선 이렇게 구현하긴 했는데, 주노는 어떠셨나요?

Copy link
Member Author

@Choi-JJunho Choi-JJunho Oct 10, 2023

Choose a reason for hiding this comment

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

@Transactional을 코드로 구현한다면 이런 느낌이지않을까? 라는 생각에 저는 이질감이 딱히 없었습니다 😅
흑마법(리플랙션)을 사용하지 않는 방법중에서는 최선이라는 생각입니당

Copy link

Choose a reason for hiding this comment

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

아무래도 그런 것 같죠? ㅎㅎ

Comment on lines +11 to +12
public TxUserService(UserService userService, TransactionExecutor transactionExecutor) {
this.userService = userService;
Copy link

Choose a reason for hiding this comment

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

UserService 인터페이스로 받기 vs AppUserService 구현체로 받기

전자의 경우, 인자로 TxUserService가 들어오면 transactionExectuor의 메서드들이 여러 번 호출되는 문제가 생길 수 있을 것 같아요.

자주 발생할 경우 같지는 않아서 '굳이 그렇게 할 필요가 있나?'라는 고민도 드는데, 주노라면 어떤 선택을 하실건가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

저는 굳이 그렇게 할 필요가 있나?라는 입장으로 이정도는 개발자의 책임의 영역이라고 생각해도 무관할 것 같습니다 😅

Copy link

@0chil 0chil Oct 10, 2023

Choose a reason for hiding this comment

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

저도 동의합니다.
라이브러리를 개발하는 입장에서
타입을 특정 구현체로 제한시켜 유연성을 희생하기보다는, 이 정도는 사용자 책임으로 남기는 것이 좋을 것 같아요.

어떤 사용자는 정말 TxUserService를 중첩해서 만들고 싶을 수도 있죠!

좋은 답변 감사합니다!

Comment on lines +24 to +28
Map<DataSource, Connection> connectionWithDataSource = resources.get();
if (connectionWithDataSource == null) {
connectionWithDataSource = new HashMap<>();
resources.set(connectionWithDataSource);
}
Copy link

Choose a reason for hiding this comment

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

잘 해주셨습니다! 👍👍👍💯💯💯

Copy link

@0chil 0chil left a comment

Choose a reason for hiding this comment

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

안녕하세요 주노! 빠른 리뷰 반영 감사합니다!
정말 잘 구현해주셔서 이게 마지막 리뷰가 될 것 같아요.

마지막으로 멀티 쓰레딩 환경에 관련한 질문 남겼읍니다!

Comment on lines 52 to 53
final var appUserService = new AppUserService(userDao, userHistoryDao);
final var userService = new TxUserService(appUserService, new TransactionExecutor(transactionManager));
Copy link

Choose a reason for hiding this comment

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

JdbcTemplate의 Connection을 DatasourceUtils를 이용하여 가져오도록 수정했습니다. 이 과정에서 try-with-resource의 대상에서 제외시켰습니다.

확인했습니다. 이제 DataSourceUtils 가 커넥션을 열고 닫는 책임을 가지겠네요.

추가로 Transaction 수행 과정에서 TransactionManager가 Connection을 필드로 가짐으로써 close된 Connection이 계속 남아있던 부분을 확인했습니다.
Transaction begin 시 Connection이 close되어있다면 connection을 새롭게 갱신하는 로직을 추가했습니다.

고생하셨습니다~! 이제 닫힌 커넥션을 갱신할 수 있게 되었네요!

하지만 이 방법은 임시방편이라는 생각이 들어요.
TransactionManager의 인스턴스를 정말 하나만 만들어도 될까요?
저는 주노 코드에서 TransactionManager 객체와 '논리적 트랜잭션' 이라고 부르는 그것이 생명 주기를 같이 한다고 느껴집니다.
그래서 현재 상태로는 여러 쓰레드가 하나의 논리적 트랜잭션을 사용하는 꼴이 되므로 멀티쓰레드 환경을 지원하지 못할 것이라는 의심이 들어요.

멀티 쓰레드 환경으로 가면,
여러 쓰레드가 Service 객체 -> TransactionExecutor 객체 -> TransactionManager 객체를 공유해 사용할 거에요.
( -> 는 객체 참조입니다!)

정말 그렇게 되어도 괜찮을까요?

Copy link

@0chil 0chil left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다 주노!
다음 미션도 화이팅입니닷!

@0chil 0chil merged commit b4fd663 into woowacourse:choi-jjunho Oct 11, 2023
1 check failed
@Choi-JJunho Choi-JJunho self-assigned this Oct 12, 2023
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