-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix/#182: 공지사항 크롤링 비동기처리 추가 및 무한 슬랙알림 문제 해결 #184
Conversation
비동기처리 이외에도 기존에 함수단위의 트랜잭션을 학과 단위의 트랜잭션으로 축소
만약 도중에 탈출하지 않으면 무한 루프로 인해 서버에 안좋은 영향 또는 무한 슬랙 알림을 받기에 3회까지 시도 후 종료되도록 설정 추가적으로 크론 내부에서 트랜잭션을 생성하지 않도록 변경
src/db/data/noticeHandler.ts
Outdated
const connection = await db.getConnection(); | ||
await connection.beginTransaction(); | ||
try { | ||
console.log(college.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래 catch 문에서 크롤링 시 에러가 발생한 학과에 대한 정보를 슬랙 알림으로 보낼 때 college.id
를 사용하고 있으니, 콘솔문은 지워도 되지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기는 개발할때 편의를 위해 계속 넣어둔건데
크게 필요하지 않으니 빼도록 할게요!
src/db/data/noticeHandler.ts
Outdated
); | ||
const noticeLinksInDB = noticeDataInDB.map((noti) => noti.link); | ||
const pinnedNoticeLinksInDB = noticeDataInDB | ||
.filter((noti) => noti.rep_yn === true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noti.rep_yn
이 이미 boolean
값이니
filter((noti) => noti.rep_yn)
이렇게 더 간결하게 사용할 수 있을 것 같아요
src/db/data/noticeHandler.ts
Outdated
if (normalNotices.length === 0) { | ||
notificationToSlack(`${noticeLink} 크롤링 실패`); | ||
connection.release(); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early return 패턴을 적용중인 것 같은데, notmalNotices
를 참조하는 바로 아래에서 적용하는 건 어떨까요?
const normalNotices = noticeLists.normalNotice;
if (normalNotices.length === 0) {
notificationToSlack(`${noticeLink} 크롤링 실패`);
connection.release();
return;
} | ||
} | ||
}); | ||
|
||
await Promise.all(savePromises); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 Promise.all
을 사용해서 비동기를 병렬로 처리하니 시간이 훨씬 줄어들었네요.
기존에는 n시 19분 쯤에 항상 알림이 왔던 것 같은데, 최근에는 4분정도에 오더라고요
불필요한 콘솔 로그 제거 얼리 리턴 위치 변경 필터 조건 간결화
반영 완료했어요! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM
🤠 개요
💫 설명
크론 동작 시 3회 이상 계속 에러가 발생 시 재귀를 호출하지 않고 반환하도록 수정
기존에는 cron 함수 동작 시 예기치 못한 에러가 발생하면 성공할때 까지 재귀하도록 설정해뒀어요. 하지만 이전에 재귀를 하면서 문제를 해결하지 못해 결국 무한 루프에 돌게되는 문제가 발생하면서 무한 슬랙알림을 받게되는 문제가 있었어요.
그렇기에 cron 함수의 경우 최대 3회 재귀를 동작한 후 그래도 에러가 발생한다면 해당 함수는 return 하게 하여 무한 루프가 동작하지 않도록 했어요!
공지사항을 비동기처리로 처리
#34 에서 처리했던 학과 공지사항 핸들링을 비동기로 처리한것과 비슷해요
기존 20분 가량 걸리던 학과 공지사항 크롤링을 비동기 처리를 통해 4분 이내에 처리되도록 개선했어요
트랜잭션 범위 축소
트랜잭션의 단위는 작을수록 좋은걸로 들었어요 그 이유는 쿼리문 결과를 바로 데이터베이스에 적용되지 않고 메모리에 저장하고 있다가 commit 시에 한 번에 동작하기에 트랜잭션에 많은 로직이 들어있을수록 RAM 사용량이 커지게 되고 서버와 DB에게 좋지 않은 영향을 끼쳐요
그렇기에 트랜잭션의 단위를 각 학과별로 뒀어요. 그렇기에 학과별 트랜잭션을 만들어 문제가 생기면 해당 학과만 크롤링 반영이 되지 않도록 했어요
고정 공지사항 처리 로직 변경
이전에는 모든 학과 공지사항을 일괄적으로 일반 공지사항으로 변경한 뒤 고정 공지사항인 경우 rep_yn 을 true 로 업데이트해주는 방식이었어요. 하지만 이렇게 동작하다보니 고정 공지사항에 대해 제대로 처리되지 못한 학과는 고정 공지사항이 없어지는 문제가 있기에 로직을 개선했어요
DB에 저장된 고정 공지사항이 크롤링된 고정 공지사항에 존재하지 않는다 -> 해당 DB에 저장된 공지의 rep_yn = false 로 바꿔 일반 공지로 변경
크롤링된 고정 공지사항이 DB에는 일반 공지로 저장되어 있다 -> 해당 DB에 저장된 공지의 rep_yn = true 로 바꿔 고정 공지로 변경
📷 스크린샷 (Optional)