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단계] 성하(김성훈) 미션 제출합니다. #489

Merged
merged 12 commits into from
Oct 11, 2023

Conversation

sh111-coder
Copy link

안녕하세요 루쿠! 빠르게 리뷰해주셔서 4단계 구현할 수 있었어요!! 👍🏻👍🏻

이번에는 미션 요구사항에 나온대로 구현을 진행해봤어요!

  1. Connection을 파라미터로 받는 서비스의 로직들을 제거하고, DataSourceUtil & TransactionSynchronizationManager을 사용하여 Connection 동기화
  2. 트랜잭션 서비스 추상화
  • UserService -> Impl : AppUserService / TxUserService
  • TxUserService에서 트랜잭션 처리 로직 중복 -> TransactionExecutor을 사용하여 Service 로직을 함수형 인터페이스로 받아서 처리
    • Return Type 존재 여부에 따라서 execute 메소드 오버로딩해서 함수형 인터페이스를 다르게 받아서 처리

@sh111-coder sh111-coder requested a review from aiaiaiai1 October 7, 2023 08:40
@sh111-coder sh111-coder self-assigned this Oct 7, 2023
Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하~
4단계 빠르게 구현해주셨네요!
몇가지 의견과 코멘트 남겨드렸는데 확인부탁드려요~

Comment on lines 14 to 19
public static Connection getResource(DataSource key) {
return null;
if (resources.get() == null) {
return null;
}
Map<DataSource, Connection> connectionMap = resources.get();
return connectionMap.get(key);

Choose a reason for hiding this comment

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

맨 첫줄에서 Map<DataSource, Connection> connectionMap = resources.get(); 를 실행하고
if 문에서 connectionMap의 값이 null인지를 확인하면 가독성이 더 좋아질거같아요!

Copy link
Author

Choose a reason for hiding this comment

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

확실히 그렇네요!! 수정했습니다!!

Comment on lines 22 to 29
public static void bindResource(DataSource key, Connection value) {
Map<DataSource, Connection> connectionMap = new HashMap<>();
Connection mappedConnection = connectionMap.put(key, value);
if (mappedConnection != null) {
throw new IllegalStateException("fail to bind resource because of already exist bound thread");
}
resources.set(connectionMap);
}

Choose a reason for hiding this comment

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

bind 할때마다 새로운 map을 만들어서 resources에 set 해주면은 기존에 resources에 존재했던 DataSource에 대한 Connection의 map 정보가 사라지게 될거같아요!

그리고 if문을 통해 mappedConnection 의 값이 존재하면 예외를 터트리고 있는데
bind 할 Connection은 매번 값이 존재해서 예외가 무조건 발생하게 되지 않나요?! 성하의 의도가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

bind 할때마다 새로운 map을 만들어서 resources에 set 해주면은 기존에 resources에 존재했던 DataSource에 대한 Connection의 map 정보가 사라지게 될거같아요!

해당 부분은 수정했습니다!!

그리고 if문을 통해 mappedConnection 의 값이 존재하면 예외를 터트리고 있는데
bind 할 Connection은 매번 값이 존재해서 예외가 무조건 발생하게 되지 않나요?! 성하의 의도가 궁금합니다!

이 부분은 Connection mappedConnection = connectionMap.put(key, value);에서 Map의 put의 값을 받고 있는데요!
Map에서 put 메소드의 리턴 값의 의미는 다음과 같습니다!

the previous value associated with key, or null if there was no mapping for key. (A null return can also indicate that the map previously associated null with key, if the implementation supports null values.)

요약하면, 이전에 맵에 key에 해당하는 값이 존재한다면 해당 값을 반환하고, 존재하지 않았다면 null을 반환합니다!
따라서 mappedConnection이 null이 아니라면 이미 스레드에 리소스가 bind 되어 있다는 것을 의미하므로
예외를 던져주게 구현했습니다!!

그래서 put을 하기 전에 이미 존재하는 Connection이 존재한다면 예외를 발생시키도록 했습니다!

Choose a reason for hiding this comment

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

아하 그렇군요! 알려주셔서 감사합니다~

Comment on lines 31 to 37
public static Connection unbindResource(DataSource key) {
return null;
Map<DataSource, Connection> connectionMap = resources.get();
Connection connectionToRemove = connectionMap.get(key);
resources.remove();
return connectionToRemove;
}
}

Choose a reason for hiding this comment

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

여기서도 connectionMap의 사이즈가 0일때 resources.remove() 하는게 더 좋아보입니다!

Copy link
Author

Choose a reason for hiding this comment

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

넵!!! 그렇네요 ㅠㅠ
수정했습니다!!

Comment on lines 18 to 61
public void execute(final ServiceExecutor serviceExecutor, final boolean isReadOnly) {
Connection connection = DataSourceUtils.getConnection(dataSource);

try {
connection.setAutoCommit(false);
connection.setReadOnly(isReadOnly);
serviceExecutor.doInAction();

connection.commit();
} catch (SQLException e) {
try {
connection.rollback();
} catch (SQLException ex) {
throw new DataAccessException(ex);
}
} finally {
DataSourceUtils.releaseConnection(connection, dataSource);
TransactionSynchronizationManager.unbindResource(dataSource);
}
}

public <T> T execute(final ServiceCallback<T> serviceExecutor, boolean isReadOnly) {
Connection connection = DataSourceUtils.getConnection(dataSource);

try {
connection.setAutoCommit(false);
connection.setReadOnly(isReadOnly);

T result = serviceExecutor.doInAction();

connection.commit();
return result;
} catch (SQLException e) {
try {
connection.rollback();
} catch (SQLException ex) {
throw new DataAccessException(ex);
}
} finally {
DataSourceUtils.releaseConnection(connection, dataSource);
TransactionSynchronizationManager.unbindResource(dataSource);
}
return null;
}

Choose a reason for hiding this comment

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

반환타입이 있는것과 없는 것으로 메서드를 나누어주었네요!
중복을 제거하는 다른 방법은 없을까요?? 🤔
TxUserService에서 void 메서드를 람다식으로 넘겨서 호출할때
람다식에 return null을 같이 넘겨주어 <T> T execute 한개의 메서드만 사용하는건 어떤것 같나요??!

Copy link
Author

@sh111-coder sh111-coder Oct 9, 2023

Choose a reason for hiding this comment

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

개선 방법 제안까지 해주셔서 감사합니다!! 😊
중복이 발생하긴 하지만, null을 넘겨주는 것 보다는 오버로딩이 개인적으로 더 좋은 것 같아서 이대로 놔두려고 하는데 괜찮을까요?!

Choose a reason for hiding this comment

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

넵 좋습니다 👍

this.dataSource = dataSource;
}

public void execute(final ServiceExecutor serviceExecutor, final boolean isReadOnly) {

Choose a reason for hiding this comment

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

readOnly 옵션도 추가해주셨네요! 👍

}

public static Connection unbindResource(DataSource key) {
return null;
Map<DataSource, Connection> connectionMap = resources.get();
Connection connectionToRemove = connectionMap.get(key);

Choose a reason for hiding this comment

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

connectionMap이 null 일 수 도 있을거같네용

Comment on lines 34 to 35
DataSourceUtils.releaseConnection(connection, dataSource);
TransactionSynchronizationManager.unbindResource(dataSource);

Choose a reason for hiding this comment

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

개인적으로 dataSource에 대한 connection을 unbind하고 connection을 close하는 순서가 적절하다고 느껴지는데
메서드 순서를 변경해주는건 어떤가요?!

Copy link
Author

Choose a reason for hiding this comment

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

아!! 순서 변경하는게 좋은 것 같아요!!! 수정했습니당! 👍🏻👍🏻

Comment on lines 7 to 15
public class TxUserService implements UserService {

private final UserService userService;
private final TransactionExecutor transactionExecutor;

public TxUserService(final UserService userService) {
this.userService = userService;
this.transactionExecutor = new TransactionExecutor(DataSourceConfig.getInstance());
}

Choose a reason for hiding this comment

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

생성자 파라미터로 혹여나 TxUserService와 같은 의도하지 않은 UserService 구현체가 들어갈수도 있을거같아요
좀더 구체적인 파라미터로 제한해주는건 어떤것같나요??

Copy link
Author

Choose a reason for hiding this comment

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

좋은 것 같아요!! 수정했습니다! 👍🏻👍🏻

@sh111-coder
Copy link
Author

리뷰 감사합니다 루쿠!! 좀 급하게 4단계 작성해서 TransactionSynchronizationManager 구현 부분에
허점이 좀 많았던 것 같아요!!
남겨주신 코멘트 기반으로 리팩토링 진행했습니다!!

Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하! 피드백 반영해주신거 확인했어요!
다만 한가지 걸리는 부분이 있는거 같아서 RC 보내봅니다..!
다음 요청때 바로 머지하도록 할께요~

Comment on lines 29 to 32
Connection mappedConnection = connectionMap.put(key, value);
if (mappedConnection != null) {
throw new IllegalStateException("fail to bind resource because of already exist bound thread");
}

Choose a reason for hiding this comment

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

현재 connectinMap에서 값을 넣고 올바르지 않으면 예외를 터트려주고있지만 잘못된 값은 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 mappedConnection = connectionMap.put(key, value);에서 나오면
mappedConnection은 해당 key에 해당하는 value가 존재한다면 존재하는 value를 반환하고,
key에 해당하는 value가 없다면 Map에 key, value를 넣고 null이 들어가게 됩니다!

그래서 예외를 발생시키는 이유가, 이미 ConnectionMap에 DataSource에 해당하는 Connection이 존재한다면
이미 스레드에 bind된 Resource가 있는 것이므로 예외를 발생시켰습니다!

그래서 결론적으로 put을 한 결과가 if 검증에서 필요해서 순서는 저대로 해야하는 것 같아요!

Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하!
피드백 반영하시느라 고생하셨어요 ㅎㅎ
미션 여기서 마무리할께요! 다음 미션도 화이팅입니다~ 🚀

@aiaiaiai1 aiaiaiai1 merged commit edbfac6 into woowacourse:sh111-coder 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