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단계] 쥬니(전정준) 미션 제출합니다. #516

Merged
merged 9 commits into from
Oct 10, 2023

Conversation

cpot5620
Copy link

@cpot5620 cpot5620 commented Oct 8, 2023

안녕하세요 루카 ~
주말 내내 테코톡 준비도 안 하고 쉬다가... 축구 가기전에 급하게 미션 제출합니다 ⚽

미션 진행하면서 한 가지 고민인 부분이 생겼는데요.
아래쪽에 남겨놓을게요 !

public class QueryTemplate {
    // 생략
    public <T> T update(String sql, UpdateExecutor<T> executor, Object... args) {
        Connection connection = DataSourceUtils.getConnection(dataSource);
        try (PreparedStatement preparedStatement = getInitializedPreparedStatement(sql, connection, args)) {
            return executor.execute(preparedStatement);
        } catch (SQLException e) {
            log.error(e.getMessage(), e);
            throw new DataAccessException(e);
        }
    }
}

위 코드에서 Connection 객체 사용 후, 반환하는 부분에 대해서 고민이 있습니다.

트랜잭션을 사용하는 곳에서는 DataSourceUtils.releaseConnection 메서드를 통해 해당 객체를 반환해주는데요.
트랜잭션을 사용하지 않을 경우에는 해당 객체를 반환하기 애매하더라구요.
try-with-resources를 사용하면, 트랜잭션을 정상적으로 처리할 수 없기 때문에요 ㅠㅠ

제가 놓치고 있는 부분이 있을까요 ?

@kpeel5839
Copy link

어쩔 tv

@e-astsea
Copy link
Member

e-astsea commented Oct 8, 2023

저쩔 tv

Copy link

@dooboocookie dooboocookie left a comment

Choose a reason for hiding this comment

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

아니 쥬니!
이번 코드 진짜 너무 멋있는데요???

배우면서 잘 봤습니다.

코드레벨에서 딱히 지적할만한 부분이 없었어요.
그래서 몇가지 질문과 사소한 변경사항 남기겠습니다.

테스트는 없긴 하지만, 이번 미션 같은 경우는 User에 대한 테스드들로 충분히 라이브러리가 잘 작동하는지 테스트 가능했다고 생각합니다.

질문 답변

위의 경우는 그냥 JdbcTemplate에서 사용하는 영역 같은데요.
지금 현재 저 영역만 들여다 봤을 때는,
connection.setAutoCommit(false)를 따로 호출하지 않는 상황 같습니다.

마찬가지로 그냥 JdbcTemplate을 사용하지 않고 직접 스테이트먼트로 쿼리 호출해도, 따로 오토커밋 모드를 꺼놓지 않았기 때문에, 일반적인 처리를 할 것 같습니다.

만약 그 메서드들을 호출하는 입장에서 트랜잭션 처리를 하고 있다면 그 커넥션을 쓸테니 그 룰을 받아드리겠죠?

그럼 여기서 궁금해져야되는 점은 저는 만약 트랜잭션을 안열었을 떄가 아니라
트랜잭션 안에서 트랜잭션을 열면 어떻게 되는가인것 같아요!
쥬니가 작성한 라이브러이에서 트랜잭션안에서 트랜잭션을 열어서 트랜잭션 전파 수준같은 것을 조절할 수 있을까요?(구현해달라는 의미는 아니고 고려해보면 좋을것 같아요!)

사실상 머지해도 상관 없는 코드 같은데, 그래도 쥬니 리뷰어여서 행복했었어서... 한번만 더 핑퐁하죠...

Comment on lines 17 to 20
@Override
public User findById(long id) {
return appUserService.findById(id);
}

Choose a reason for hiding this comment

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

P?:

이 부분은 트랜잭션 처리를 안하신 이유가 따로 있으신가요??

Copy link
Author

Choose a reason for hiding this comment

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

이 부분에 대해서 질문하실거라고 생각했어요 ㅎㅎ

조회용 트랜잭션(readOnly)을 만들기 위해서, 부가적인 기능을 구현해야한다는 점에서 귀찮음을 느꼈어요..
하지만, 빠르게 구현했습니당 ~!

Comment on lines 17 to 33
public void start() {
transactionTemplate.execute(connection -> connection.setAutoCommit(false));
}

public void commit() {
transactionTemplate.execute(Connection::commit);
close();
}

private void close() {
transactionTemplate.execute(connection -> DataSourceUtils.releaseConnection(connection, dataSource));
}

public void rollback() {
transactionTemplate.execute(Connection::rollback);
close();
}

Choose a reason for hiding this comment

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

👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏
여기랑 TransactionTemplate 구현 너무 깔끔해서 기립박수 쳤어요!!
트랜잭션에게 어떤 동작 시킬 때 항상 DataSourceUtils에서 싱크된 커넥션 얻어온다는 아이디어군요!!
전 왜 이런생각을 못할까요!!
이번 단계 제대로 소화하신것 같군요

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 33 to 35
Connection removedConnection = connections.remove(key);
resources.remove();

Choose a reason for hiding this comment

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

P?:
Map의 값을 비워주신다음에
쓰레드 로컬 변수까지 클리어해주시는 이유가 있으신가요??

Copy link
Author

Choose a reason for hiding this comment

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

쓰레드 로컬 변수까지 제거할 필요는 없었던 것 같아요 !

해당 사항 반영했습니다 !
추가적으로, bind 할 때에도 로컬 변수가 null인 경우에만 새롭게 할당하는 방식으로 수정했습니다.

Comment on lines 22 to 46
@Override
public void insert(User user) {
transactionManager.start();
try {
appUserService.insert(user);

transactionManager.commit();
} catch (DataAccessException e) {
transactionManager.rollback();
throw e;
}

}

@Override
public void changePassword(long id, String newPassword, String createBy) {
transactionManager.start();
try {
appUserService.changePassword(id, newPassword, createBy);

transactionManager.commit();
} catch (DataAccessException e) {
transactionManager.rollback();
throw e;
}

Choose a reason for hiding this comment

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

P3:

지난 번 리뷰에서도 말씀 드렸었는데,
이부분의 try-catch 중복 제거하는 거 어떻게 생각하시나요?

지금은 Service도 몇개 안되고 그안에 메서드들도 몇개 안되지만 이게 많아지면 라이브러리 사용자가 너무 구현하기 힘들 것 같아요.
그리고 혹은 이런 라이브러리 사용자가 직접 트랜잭션을 감싸는 방법 말고,
mvc에서 사용했던 어노테이션 같은 방법으로 트랜잭션 처리하는 방법은 생각해보셨나요??

Copy link
Author

Choose a reason for hiding this comment

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

try-catch 중복은 제거 완료했습니다.

어노테이션 방법으로 구현해보고 싶어서, 한 시간정도 시도해보았는데요.
구현해야 할 클래스 및 기능들이 점점 많아지고.. 이게 맞나 싶어서 다 롤백했습니다 ㅠㅠ (너무 어렵기도 했어요..)
가능하시다면, 루카가 구현해서 설명해주시면 좋을 것 같아요 ㅋㅋ

@cpot5620
Copy link
Author

cpot5620 commented Oct 9, 2023

아 증말 나 좀 그만 좋아해요.
왜 이렇게 귀찮게 하는거야 _ 🥸

각 코멘트에 의견 및 반영사항 남겼으니, 확인 부탁드려요 루카 !

Copy link

@dooboocookie dooboocookie 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 17 to 21
public void start(boolean readOnly) {
transactionTemplate.execute(connection -> {
connection.setAutoCommit(false);
connection.setReadOnly(readOnly);
});

Choose a reason for hiding this comment

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

좋은데요?

@dooboocookie dooboocookie merged commit 5ad323b into woowacourse:cpot5620 Oct 10, 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.

4 participants