feat [mumu, redis]: threadTs 저장소를 in-memory Map에서 Redis로 마이그레이션#14
feat [mumu, redis]: threadTs 저장소를 in-memory Map에서 Redis로 마이그레이션#14
Conversation
ExceptAnyone
left a comment
There was a problem hiding this comment.
제가 redis 리뷰를 다 해보네요 ㅋㅋㅋ
덕분에 좋은 경험합니다!
궁금한 것들 코멘트 남겨봤어요 확인부탁드려요.
| case "pull_request": { | ||
| const result = handlePullRequest(pullRequestSchema.parse(req.body), slackNotifier); | ||
| console.log(`Pull Request: ${JSON.parse(result)}`); | ||
| handlePullRequest(pullRequestSchema.parse(req.body), slackNotifier).then((res) => console.log(res)); |
There was a problem hiding this comment.
여기에는 catch를 안붙여도 되나요? just wonder
There was a problem hiding this comment.
하위에서 일단 주요한 비동기 액션들에 대해서는 catch를 작성해두어서 여기서 catch까지 할 필요 없다고 생각했어요.
만약 핸들링되지 않은 에러가 있다면 이미 상위 스코프에서 event 변수를 알고 있으니, 아래의 catch에서 일괄적으로 처리할 수 있다고 판단하여 요렇게 구현했습니다.
There was a problem hiding this comment.
음 그럴수잇겠네요. 제가 처음에 생각했을 때는 promise를 반환하니까 try catch 블록을 벗어나게 되는건 아닌가? 싶었는데 주용님의 일괄 처리 판단 좋았던 것 같습니다
| } | ||
|
|
||
| await redisStorage.delete(cacheKey).catch((err) => { | ||
| console.log(`${cacheKey}: Redis 캐시 삭제 실패`, err); |
There was a problem hiding this comment.
여기는 console.error가 아니라 log로 작성해주신 이유가 있나요?
There was a problem hiding this comment.
사실 큰 이유는 없고 빨간색으로 표시되는 에러 로그가 많으면 괜히 과부하가 오더라고요. redis에 쓰고 읽고하는 작업은 ttl이 부여되어 있는 이상 에러가 그리 심각한 수준이라고는 판단하지 않았어서 로그로 남겼습니다.
| message: response.message, | ||
| }); | ||
| ).catch((err) => { | ||
| console.log(`${cacheKey}: 리뷰어 지정 실패`, err); |
There was a problem hiding this comment.
여기도 console.error가 아니라 log인 점이 궁금합니다!
There was a problem hiding this comment.
요것도 동일합니당.
로그에 대한 기준은 한 번더 고민해보겠습니다.
servers/mumu/src/index.ts
Outdated
| res.status(200).json({ status: "ok" }); | ||
| }); | ||
|
|
||
| const port = process.env.PORT || 3000; |
There was a problem hiding this comment.
둘 다 3000으로 저장되어 있는데 or 연산자로 다시 지정해준 이유가 있나요 ?
There was a problem hiding this comment.
혹시나 env가 없을 경우 처리 아닐까요? 그런데 env에 두는 값인데 fallback값이 있는 것이 의도에 맞을지 생각해봐야 할 것 같은데 어떻게 생각하시나요?!
There was a problem hiding this comment.
default 값을 충분히 부여할 수 있는 변수라 undefined 타입을 제외하기 위해 3000을 넣긴 하였습니다.
서버 구동을 위해 필수 변수들은 보시다시피 assert를 통해서 타입 추론뿐만 아니라 런타임에 있음을 보장하고 있거든요.
근데 이와는 별개로 지금 string | 3000으로 추론되고 있어서 수정이 필요하겠네요 ㅋㅋㅋㅋㅋ ..
| await redisStorage.delete(cacheKey).catch((err) => { | ||
| console.log(`${cacheKey}: Redis 캐시 삭제 실패`, err); | ||
| }); | ||
| return JSON.stringify({ success: true, message: "Pull request closed." }); |
There was a problem hiding this comment.
캐시 삭제 실패해도 success: true를 반환하는 것 같은데, 의도된 동작인지 궁금해요 !
There was a problem hiding this comment.
응답에 대한 로그를 어디서 남기냐의 문제를 고민하다가 이렇게 작성되었는데, 요 구조는 한 번 고민해보고 리팩토링하겠습니다.. !
constantly-dev
left a comment
There was a problem hiding this comment.
수고하셨습니다! redis 코드를 처음보는 것 같아 신기하네요,,ㅎㅎㅎ
저도 다른 분들 의견과 비슷하고 코멘트 몇개 달았습니다! 👍
| threadTs: response.ts, | ||
| message: response.message, | ||
| }, | ||
| { ex: 60 * 60 * 24 * 21 }, |
There was a problem hiding this comment.
사실상 21일보다 긴 기간동안 열리는 PR은 없을것이라고 생각하면 되겠죠??
servers/mumu/src/index.ts
Outdated
| res.status(200).json({ status: "ok" }); | ||
| }); | ||
|
|
||
| const port = process.env.PORT || 3000; |
There was a problem hiding this comment.
혹시나 env가 없을 경우 처리 아닐까요? 그런데 env에 두는 값인데 fallback값이 있는 것이 의도에 맞을지 생각해봐야 할 것 같은데 어떻게 생각하시나요?!
| await redisStorage.delete(cacheKey).catch((err) => { | ||
| console.log(`${cacheKey}: Redis 캐시 삭제 실패`, err); |
There was a problem hiding this comment.
캐시 삭제 자체는 쓰레드 생성 여부, 위 조건 분기와 상관없이 삭제하는 것이 의도하신 부분인지 궁금합니다!
There was a problem hiding this comment.
맞습니다. threadTs가 존재함이 일단 쓰레드가 생성되었음이랑 동치의 개념은 아니거든요.
일단 thread 데이터를 가져왔으면(storage.get) pull request가 닫힌 뒤에는 항상 삭제하는 것을 의도했습니다.
위 조건분기와는 다르게 저기서 if(thread) 분기를 한 번 치고 그안에서 delete하는게 안전하긴 하겠네요 ! 감사합니다.
ExceptAnyone
left a comment
There was a problem hiding this comment.
리뷰 답글 전부 확인했습니다. 고생하셨어여!
작업 내용
@makers-devops/redis공용 패키지를 신규 생성하여 Redis 클라이언트 래핑 (register/get/set/delete)Made with Cursor