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 라이브러리 구현하기 - 1단계] 도이(유도영) 미션 제출합니다. #316

Merged
merged 7 commits into from
Sep 30, 2023

Conversation

yoondgu
Copy link

@yoondgu yoondgu commented Sep 28, 2023

안녕하세요 에코! 🚀
코드 리뷰로 만나게 되어 반갑습니다 ㅎㅎ 연휴는 잘 보내고 계신가요?
이번 단계에서는 JdbcTemplate을 통해 중복 코드를 제거하는 데에만 집중했습니다.
의문이 드는 점이 있다면 얼마든지 코멘트 또는 디엠 남겨주세요!
잘 부탁드립니다!!! 🙇‍♀️

optimize imports를 한 커밋이 함께 있어서..
커밋 범위 + 추가 커밋(조회 쿼리 단건, 다건 분리) 로 확인하시면 편하실 것 같습니다!

@yoondgu yoondgu self-assigned this Sep 28, 2023
@echo724
Copy link

echo724 commented Sep 29, 2023

안녕하세요 도이! 연휴인데도 빠르게 하셨네요 ㅎㅎ 제가 가족들을 만나는 중이라 내일 오전 내로 리뷰 해드리겠습니다~~!

Copy link

@echo724 echo724 left a comment

Choose a reason for hiding this comment

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

안녕하세요 도이~ 추석인데도 일찍 해주셨는데 늦게 리뷰 달아드려 죄송해요 ㅜ
전반적으로 잘 짜주셔서 읽고 이해하는데도 큰 문제 없이 잘 짜신것 같더라구요! 예외 처리도 잘하시구 요구사항 만족도 잘 되어있는 것 같아 부분부분 궁금한 것만 달았습니다~!
추천드리고 싶은건 ConnectionPool도 다음 스텝때 구현해보시는건 어떨까요? 학습 테스트도 있고 해서 한 번 해보시면 좋을것 같아요! 물론 요구사항은 아니였어서 추천입니다. 다음 스탭도 화이팅하시고 이번 스탭은 바로 머지하겠습니다~

Comment on lines +88 to +97
config.addDataSourceProperty("cachePrepStmts", "true");
config.addDataSourceProperty("prepStmtCacheSize", "250");
config.addDataSourceProperty("prepStmtCacheSqlLimit", "2048");
config.addDataSourceProperty("useServerPrepStmts", "true");
config.addDataSourceProperty("useLocalSessionState", "true");
config.addDataSourceProperty("rewriteBatchedStatements", "true");
config.addDataSourceProperty("cacheResultSetMetadata", "true");
config.addDataSourceProperty("cacheServerConfiguration", "true");
config.addDataSourceProperty("elideSetAutoCommits", "true");
config.addDataSourceProperty("maintainTimeStats", "false");
Copy link

Choose a reason for hiding this comment

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

오 혹시 이 설정도 하신 이유나 레퍼런스가 있으실까요?! 출처 링크만 달아주셔도 좋을것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

HikariCP 공식문서에서 추천하는 MySQL 설정값을 적용해보았습니다!
그런데 해당 테스트의 시간 측정 결과만 봤을 때에는 오차 범위가 커서 확실한 비교가 되는지는 아직 모르겠습니다 😅
https://github.com/brettwooldridge/HikariCP/wiki/MySQL-Configuration


public Connection getConnection() throws CannotGetJdbcConnectionException {
try {
DataSource dataSource = DataSourceConfig.getInstance();
Copy link

Choose a reason for hiding this comment

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

어차피 DataSourceConfig 자체가 싱글톤이기 때문에 크게 상관은 없을것 같지만 필드로 만들어 초기화하는 것은 어떨까요? getConnection()을 할때마다 추가적으로 DataSourceConfig.getInstance()가 호출되고 결국에는 같은 인스턴스를 반환받기 때문에 불필요하다는 생각이 들었습니다!

Copy link
Author

Choose a reason for hiding this comment

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

오 그러네요!! 다음 단계에서 반영하도록 하겠습니다 ㅎㅎ

private static final String WRONG_QUERY_ERROR_MESSAGE_FORMAT = "SQL Query error: %s \nSQL: %s";

public SqlQueryException(final String message, final String query) {
super(String.format(WRONG_QUERY_ERROR_MESSAGE_FORMAT, message, query));
Copy link

Choose a reason for hiding this comment

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

오옹 SqlQueryException 좋네요! 예외를 확인하기 편한 메세지 구조인 것 같습니다! 👍


import java.sql.Connection;

public interface ConnectionManager {
Copy link

Choose a reason for hiding this comment

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

ConnectionManager 인터페이스도 적절한 곳에 잘 두신 것 같네요! 역할 자체는 DataSource에서 Connection을 가져오는데 가져올때 에외가 발생하면 예외를 처리해주는 역할 같은데 맞을까요?!

Copy link
Author

Choose a reason for hiding this comment

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

네 맞습니다!
그리고 덧붙이자면,
Connection을 얻는 방법으로는 DriverManager도 있기 때문에 이를 프레임워크 단의 인터페이스로 정의했습니다.
그리고 사용자가 인터페이스를 구현해 원하는 방식으로 connection을 얻어오게 하고 싶었어요.
하지만 왠만하면 DataSource를 사용할 것 같아서.. DataSoucerConnectionManager도 프레임워크에서 제공해도 되지 않나? 싶기도 하네요 😅😅

에코라면, 해당 인터페이스의 구현체는 어느 패키지에 두실 것 같나요?

@echo724 echo724 merged commit 2136df2 into woowacourse:yoondgu Sep 30, 2023
1 check failed
Copy link
Author

@yoondgu yoondgu left a comment

Choose a reason for hiding this comment

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

감사합니다 에코! 빠르게 리뷰해주셨는데 저야 말로 답변이 늦었네요 ㅎㅎ
얼른 2단계로 찾아뵙겠습니다~~!

Comment on lines +88 to +97
config.addDataSourceProperty("cachePrepStmts", "true");
config.addDataSourceProperty("prepStmtCacheSize", "250");
config.addDataSourceProperty("prepStmtCacheSqlLimit", "2048");
config.addDataSourceProperty("useServerPrepStmts", "true");
config.addDataSourceProperty("useLocalSessionState", "true");
config.addDataSourceProperty("rewriteBatchedStatements", "true");
config.addDataSourceProperty("cacheResultSetMetadata", "true");
config.addDataSourceProperty("cacheServerConfiguration", "true");
config.addDataSourceProperty("elideSetAutoCommits", "true");
config.addDataSourceProperty("maintainTimeStats", "false");
Copy link
Author

Choose a reason for hiding this comment

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

HikariCP 공식문서에서 추천하는 MySQL 설정값을 적용해보았습니다!
그런데 해당 테스트의 시간 측정 결과만 봤을 때에는 오차 범위가 커서 확실한 비교가 되는지는 아직 모르겠습니다 😅
https://github.com/brettwooldridge/HikariCP/wiki/MySQL-Configuration


public Connection getConnection() throws CannotGetJdbcConnectionException {
try {
DataSource dataSource = DataSourceConfig.getInstance();
Copy link
Author

Choose a reason for hiding this comment

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

오 그러네요!! 다음 단계에서 반영하도록 하겠습니다 ㅎㅎ


import java.sql.Connection;

public interface ConnectionManager {
Copy link
Author

Choose a reason for hiding this comment

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

네 맞습니다!
그리고 덧붙이자면,
Connection을 얻는 방법으로는 DriverManager도 있기 때문에 이를 프레임워크 단의 인터페이스로 정의했습니다.
그리고 사용자가 인터페이스를 구현해 원하는 방식으로 connection을 얻어오게 하고 싶었어요.
하지만 왠만하면 DataSource를 사용할 것 같아서.. DataSoucerConnectionManager도 프레임워크에서 제공해도 되지 않나? 싶기도 하네요 😅😅

에코라면, 해당 인터페이스의 구현체는 어느 패키지에 두실 것 같나요?

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