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단계] 루카(백경환) 미션 제출합니다. #549

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

dooboocookie
Copy link

안녕하세요 필립!
이번 미션 드디어 마지막 단계네요!!

이번 단계에서는 트랜잭션을 꼭 유저가 생성하지 않아도 데이터 소스를 주입해주면 특정 범위 안에서는 하나의 트랜잭션만 사용하도록 하는 과정을 구현해보았는데요.

ThreadLocal의 개념이 조금 어려워서 갈피를 못잡았는데, 어떻게 다하긴 했네요 ㅎㅎ

리뷰 잘 부탁드립니다!!

Copy link

@pilyang pilyang 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 +23 to +25
if (resources.get() == null) {
resources.set(new ConcurrentHashMap<>());
}
Copy link

Choose a reason for hiding this comment

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

ThreadLocal 내부에 map을 ConcurrentHashMap<>을 통해서 해준 이유가 있을까요???

제 생각으로는 ThreadLocal은 각각의 스레드마다 저장 공간을 따로 만들어 해당 스레드에서만 사용되는 공간을 만드는것으로 생각하고 있습니다. 그런데 ConcurrentHashMap은 여러 스레드에서의 접근시 동시성을 보장하기위에 put에서 락을 걸어준다고 알고 있습니다. ThreadLocal에 맵을 생성을 하면 여러스레드에서의 동시접근 가능성이 없다고생각을 하는데, 이렇게 한 이유가 있는지 궁금합니다!!

혹시 제가 잘못알고있는거일 수 있어서 제가 틀린부분이라면 지적 부탁드립니다!

Copy link
Author

Choose a reason for hiding this comment

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

아 이 부분은 제가 고려하지 못한 부분이였네요...
제가 너무 단순하게 생각한 것 같습니다.
이 부분은 쓰레드 별로 관리되는 변수기 때문에, 한 쓰레드의 두 요청이 접근하는 경우는 없을 것 같아요 ㅠㅠ
HashMap으로 변경하겠습니다

Comment on lines +17 to +20
@Override
public User findById(final long id) {
return TRANSACTION.run(connection -> userService.findById(id));
}
Copy link

Choose a reason for hiding this comment

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

해당 부분은 저도 리뷰를 받아서 고민을 해봐야 하는 내용인데, connection을 close해주는 부분이 TRANSACTION에만 있어서, userDao, userHistoryDao등의 메서드를 그냥 호출하면 커넥션이 닫히지 않는다는 문제가 있을 수 있을것 같다고 하네요.

이부분 해결에 대해서도 고민해보고 고칠 수 있으면 고쳐봐도 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분에 대해서 문제를 인지시켜주셔서 감사합니다!!
해결 방법에 대해서 직관적으로 딱 떠오르진 않았지만,

저희 조의 코코닥 의견을 적극 반영해서, 수정해보았습니다.

JdbcTemplate과 같이 Connection을 사용하는 부분에서
트랜잭션을 활성화가 되어있지 않다면(TreadLocal에 값이 없다면) Connection을 클로즈를 한다.
라는 방법으로 해결해보았습니다!!

@dooboocookie
Copy link
Author

안녕하세요 필립!
이번에도 좋은 의견 남겨주셔서 감사합니다.
말씀해주신

userDao, userHistoryDao등의 메서드를 그냥 호출하면 커넥션이 닫히지 않는다는 문제

제 나름대로 해결해보았습니다. 완벽한 방법은 아닌 것 같지만 조금 더 고려해볼 필요가 있는 문제인 것 같습니다.

@dooboocookie
Copy link
Author

필립 선생님 지적 감사합니다 ㅠㅠㅠ

image

말씀해주신대로 Connection이 어쨰뜬 쓰레드 풀에서 가져와지거나 없으면 새로 만들어서 넣기 떄문에 안되더라구요.
그래서 트랜잭션을 꼭 열지 않아도 되는부분에 커넥션을 얻어올 수 있는 메서드를 사용했습니다.

용어는 스프링의 트랜잭셔널 전파 수준에서 사용하는 용어를 선택하였습니다.

Required == 트랜잭션이 있으면 해당 커넥션을 쓰고, 없으면 새로운 커넥션을 만들어서 지정하겠다.
Supports == 트랜잭션이 있으면 해당 커넥션을 쓰고, 없으면 따로 커넥션을 만들어서 지정하지 않겠다.

라는 의미로 사용했습니다.

그래서 JdbcTemplate에서 커넥션을 사용할 때는 XXXSupports의 메서드로 커넥션을 얻어오고 닫아줬고
Transaction같이 꼭 트랜잭션이 없으면 만들어야하는 영역은 XXXRequired의 메서드로 커넥션을 얻어오고 닫아줬습니다.

필립 선생님 마지막까지 바쁘실텐데 꼼꼼히 봐주셔서 감사해용 ㅠㅠㅠ

Copy link

@pilyang pilyang left a comment

Choose a reason for hiding this comment

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

루카 마지막까지 dm확인하고 반영해주신다고 고생하셨어요...!!!

코멘트로 용어에 대한 설명까지 해줘서 루카가 어떤식으로 문제를 해결했는지도 잘 파악할 수 있었습니다!!

이번미션 고생하셨고, 마지막 남은 미션과 마지막 스프린트까지 화이팅해서 달려요!!

(루카가 해결한 부분에 대한 리뷰는 코멘트로 남겼습니다!)

Comment on lines +29 to +36
public static void releaseConnectionForTransactionRequired(Connection connection, DataSource dataSource) {
try {
connection.close();
TransactionSynchronizationManager.unbindResource(dataSource);
} catch (SQLException ex) {
throw new CannotGetJdbcConnectionException("Failed to close JDBC Connection");
}
}
Copy link

Choose a reason for hiding this comment

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

DataSourceUtils에서 각각의 역할에 맞게 메서드를 분리해줘서 구현해주셨네요..!!

저는 해당 리뷰를 받았을때 JdbcTemplate에서 connection이 autocommit상태인지를 확인해서 외부에서 오토커밋을 관리하는지 아닌지를 통해서 닫을 수 있도록 해주었었는데 (제 커밋 보기),

루카의 구현을 보니 루카의 방법이 커넥션을 가져올때 그 역활을 명시적으로 표현해주고, 데이터소스와 커넥션을 관리하는 DataSourceUtils와 트랜젝션 싱크를 관리하는 개체인 TransactionSynchronization에 그 역할을 넘겨서 루카의 구현이 더 적절하다고 생각이 드네요..!!!


추가로 지금은 커넥션을 가져오고 release하는 모든 부분에서 신경을 써서 적절한 메서드를 사용해야하는데, 이러면 가져오는 메서드와 풀어주는 메서드가 잘 안맞으면 (개발자가 실수하면) 커넥션 관리가 힘들것같긴 하네요...

근데 이부분은 어쩔 수 없는건가 싶기도 하고,,, 혹시 시간이 된다면 어떻게 하면 실수를 줄이거나 없이 잘 사용할 수 있도록 구현이 가능한 방법이 있을지도 한번 고민해봐도 재미있을것같네요...!!

저는 생각을 해보다 일단 시간관계상 포기했습니다..ㅠㅜㅋㅋㅋㅋㅋ

@pilyang pilyang merged commit d78da52 into woowacourse:dooboocookie Oct 11, 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