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단계] 도기(김동호) 미션 제출합니다. #598

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

kdkdhoho
Copy link

안녕하세요 무민! 건강상 이슈로 이제야 4단계 제출합니다..!
무민은 건강하세요 😢

요구사항에 충실하게 구현해보았습니다.
그럼 이번에도 잘 부탁드려요 :)

@kdkdhoho kdkdhoho requested a review from parkmuhyeun October 10, 2023 04:09
@kdkdhoho kdkdhoho self-assigned this Oct 10, 2023
Copy link
Member

@parkmuhyeun parkmuhyeun left a comment

Choose a reason for hiding this comment

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

안녕하세요 도기. 몸은 좀 괜찮으신가요!?
요즘 날씨가 갑자기 추워졌는데 따뜻하게 입고 다니십셔! ❄️

드디어 마지막 단계인데 그 동안 고생 많으셨습니다!!

거의 마지막이 될 거 같은데 코멘트 한번 확인해주시고 자유롭게 반영해주시면 감사하겠습니다~

app/src/main/java/com/techcourse/dao/UserDao.java Outdated Show resolved Hide resolved
app/src/main/java/com/techcourse/dao/UserHistoryDao.java Outdated Show resolved Hide resolved
}

public static void bindResource(DataSource key, Connection value) {
Map<DataSource, Connection> resource = new HashMap<>();
resource.put(key, value);
resources.set(resource);
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 되면 원래 있던 map에 덮어 씌우지 않을까요!?

Copy link
Author

Choose a reason for hiding this comment

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

음.. 처음엔 덮어 씌어져도 괜찮겠다 했는데요.
Connection이 덮어 씌어져서 기존 Connection이 날라가면 트랜잭션 유지가 안되는 문제가 생길 것 같아서, 예외를 발생하는 쪽으로 수정하겠습니다!

Copy link
Member

@parkmuhyeun parkmuhyeun left a comment

Choose a reason for hiding this comment

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

RC 요청

@kdkdhoho
Copy link
Author

항상 감사합니다 무민 :)
그런데 하나 질문이 생겼어요! TransactionSynchronizationManager에서 현재 ThreadLocal의 제네릭 타입으로 Map<DataSource, Connection을 받고 있잖아요?
그런데 현재 구조에서는 DataSource가 애플리케이션 통틀어 static 변수로 1개만 사용하고 있기 때문에 'ThreadLocal의 제네릭 타입으로 그냥 Connection만 가지면 안되나?'라고 생각했어요.
어차피 ThreadLocal에 할당하는 자원은 쓰레드별로 다르기도 하고, 현재는 DataSource가 전역변수로 1개이기 때문이죠.

어떤 이유에서 기본으로 제공되는 코드가 Map<DataSource, Connection일까요?
단일 스프링 애플리케이션에서 동시에 2개 이상의 DataSource를 사용할 수 있기 때문일까요?

Copy link
Member

@parkmuhyeun parkmuhyeun left a comment

Choose a reason for hiding this comment

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

안녕하세요 도기! 우선 마지막까지 피드백 반영 감사합니다.

어떤 이유에서 기본으로 제공되는 코드가 Map<DataSource, Connection일까요?
단일 스프링 애플리케이션에서 동시에 2개 이상의 DataSource를 사용할 수 있기 때문일까요?

예를 들어, 서로 다른 도메인에서 DB를(mysql, redis) 분리 해서 사용하고 하나의 트랜잭션 안에서 동시에 사용할 일이 생긴다면 동시에 다른 Datasource를 사용해서 작업하기 때문에 TransactionSynchronizationManager를 이용해서 좀 더 편리하게 쓰는 것 같습니다!

4단계까지 너무 고생 많으셨고 다음 미션 드디어 마지막인데요..
정말 마지막인만큼 불태우시고 화이팅하십셔 😎

이번 미션은 여기서 머지하겠습니다!

Comment on lines +30 to +35
if (Objects.isNull(connection)) {
Map<DataSource, Connection> resource = new HashMap<>();
resource.put(key, value);
resources.set(resource);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

현재는 없을 때만 map을 생성해서 넣고 있는데 있을 때도 넣어주면 좋을 것 같아요!

@parkmuhyeun parkmuhyeun merged commit cc7418c into woowacourse:kdkdhoho 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