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

snu4t에 강의 평점 동기화 #111

Merged
merged 8 commits into from
Aug 27, 2024
Merged

snu4t에 강의 평점 동기화 #111

merged 8 commits into from
Aug 27, 2024

Conversation

asp345
Copy link
Member

@asp345 asp345 commented Jul 16, 2024

snu4t mongodb에 강의평점 정보를 동기화하는 배치를 만들었고 EvaluationService에서도 snu4t mongodb를 연결해서 강의평 변동 시 같이 업데이트되게 만들었습니다.
만들고 보니 직접 mongo 연결하는게 별로 자연스러운거 같지 않긴 한데 리뷰해주시면 반영하겠습니다.

@asp345 asp345 requested review from PFCJeong and a team as code owners July 16, 2024 12:59
@asp345 asp345 requested review from davin111 and Hank-Choi July 16, 2024 12:59
Copy link
Contributor

@Hank-Choi Hank-Choi left a comment

Choose a reason for hiding this comment

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

사소한거 몇가지 코멘트남깁니다.


@Bean
fun ratingSyncJob(job: JobRepository, jobRepository: JobRepository, lectureRepository: LectureRepository): Job {
lectureIdtoLectureRatingMap = lectureRepository.findAllRatings().associateBy { it.id }.toMutableMap()
Copy link
Contributor

Choose a reason for hiding this comment

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

job에서 repository를 바로 의존성 가지는게 좀 어색한 것 같긴합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

var 로 안하고 val 로 두고 선언 시점에 바로 받아와도 괜찮을듯요

Copy link
Contributor

Choose a reason for hiding this comment

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

by lazy로 받아와도 괜찮을 것 같고요

Copy link
Contributor

Choose a reason for hiding this comment

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

근데 이거 findAllRatings가 은근히 무거울 것 같은데 어느정도 걸려요?
reader에서 불러온 후에 processor에서 chunk 사이즈마다 ratings 불러오는 것도 괜찮을 것 같네요
지금 프로젝트에 데이터가 많이 없고 무거운 쿼리도 많긴한데... 대규모 프로젝트라면 DB 부하 줄이기 위해 chunk해서 불러올 것 같긴 합니다.

Comment on lines 409 to 416
mongoTemplate.updateMulti(
Query(Criteria.where("_id").`in`(snuttId)),
Update().set("evInfo.evId", lectureRatingDao?.id)
.set("evInfo.avgRating", lectureRatingDao?.avgRating)
.set("evInfo.count", lectureRatingDao?.count),
"lectures",
).subscribe()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mongo쪽 쓰는건 service 모듈화 해서 따로 빼면 의존성 관리하기 더 쉬울 것 같아요
사실 spring event 리스너 이런거 다는 것도 생각해볼만한데 아직 그럴 필요는 없을 것 같네요

@asp345
Copy link
Member Author

asp345 commented Aug 16, 2024

말씀해주신 부분 반영해서 chunk로 나누어 처리하게 수정했는데 processor는 item 하나 단위로 처리가 돼서 processor 없이 writer에서 chunk마다 rating 불러오게 만들었습니다
그리고 강의 동기화할때 같이 돌아가게 만들면 좋을거 같아서 그 부분도 추가했습니다

@asp345 asp345 force-pushed the feature/snu4t-rating-sync branch from 81d4118 to 748fc6f Compare August 17, 2024 06:58
@asp345 asp345 force-pushed the feature/snu4t-rating-sync branch from 748fc6f to 312a89c Compare August 17, 2024 07:18
@asp345
Copy link
Member Author

asp345 commented Aug 17, 2024

processor 없이 처리하는게 훨씬 빨라서 chunk 사용하는거 자체는 유지하되 크기를 100만으로 해두었습니다

Copy link
Contributor

@Hank-Choi Hank-Choi left a comment

Choose a reason for hiding this comment

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

LGTM입니다.

@@ -78,6 +79,7 @@ class SnuttLectureSyncJobConfig(
),
),
)
.next(ratingSyncJobStep(jobRepository))
Copy link
Contributor

Choose a reason for hiding this comment

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

좋네요

@asp345 asp345 merged commit cc9c988 into develop Aug 27, 2024
2 checks passed
@asp345 asp345 deleted the feature/snu4t-rating-sync branch August 27, 2024 10:49
@asp345 asp345 restored the feature/snu4t-rating-sync branch August 31, 2024 04:34
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