[refactor] LLM 재시도 루프를 공통 executor로 추출 - #166
Conversation
댓글·답글 생성 서비스에 같은 모양의 재시도 루프가 두 벌 있고, 카드 경로에 재시도를 붙이면 네 벌이 된다. 그 루프를 담을 LlmRetryExecutor를 먼저 만든다. 시도별 토큰 합산과 GenerationLog 기록은 executor로 가져오지 않는다. 검증에 실패한 시도도 호출은 됐으니 과금되는데, 선언적 재시도는 시도 사이에 끼어들 자리가 없어 과소 집계를 막을 수 없다. 대신 call에 시도 번호를 넘기고 실패 시점마다 콜백을 열어 호출자가 계속 담당하게 한다. 백오프 대기는 RetrySleeper로 분리해 테스트가 실제로 잠들지 않게 했다. 현재 RETRY_BACKOFF_MILLIS가 0이라 아직 아무도 대기하지 않는다.
거의 같은 재시도 루프가 두 벌 있던 것을 LlmRetryExecutor 호출로 바꿨다. 남은 차이는 부르는 generator·validator·GenerationType 세 가지뿐이라, 이제 두 경로의 차이가 한눈에 보인다. 시도별 토큰 합산은 그대로 이 서비스에 남겼다. 검증에 실패한 시도도 호출은 됐으니 과금되기 때문에, executor가 넘겨주는 시도 번호와 실패 콜백 위에서 호출자가 계속 누산한다. 세 군데로 흩어져 있던 생성 로그 기록은 recordGeneration 으로 모았다. 재시도 대상이 아닌 예외를 다루는 경로에는 테스트가 없었다. 이제 그 처리가 콜백 한 줄이라 빠뜨려도 컴파일과 기존 테스트가 모두 통과해버리므로, 댓글·답글 각각에 실패 로그가 남는지 검증하는 테스트를 더했다. 동작은 그대로다 — 시도 2회, 백오프 0. 기존 테스트를 고치지 않고 통과한다.
감정 분류·한 줄 생성 두 곳을 LlmRetryExecutor 호출로 바꿨다. 재시도 횟수는 CARD_MAX_ATTEMPTS = 1 이라 호출 횟수는 지금과 같다 — 시도를 늘리려면 한 요청이 LLM을 두 번 순차 호출하는 구간이라 nginx proxy_read_timeout 까지 다시 계산해야 해서 #162 로 미룬다. 하드코딩돼 있던 attemptCount = 1 을 executor 가 넘겨주는 시도 번호로 바꿨다. 지금은 같은 값이고, 시도를 늘리면 저절로 맞는 값이 남는다. TokenUsageAccumulator 를 llm/generation 으로 옮겨 카드 경로도 쓰게 했다. 시도가 1회인 지금은 값이 달라지지 않지만, 누산기 없이 시도만 늘리면 마지막 시도의 토큰만 기록돼 과금이 과소 집계된다 — 댓글에서 이미 한 번 났던 사고라 구조를 먼저 맞춰둔다. 호출 자체가 실패해 토큰이 없는 경우 generation_log 에 null 대신 0 이 남는다. 누산기를 거치면서 생긴 유일한 값 변화이고, 댓글 경로가 이미 그렇게 남기고 있어 두 경로가 같아진다(합계 집계에는 영향이 없다). 재시도 대상이 아닌 예외에 대한 콜백은 넘기지 않는다. 카드 경로는 그런 예외를 기록하지도 상태를 되돌리지도 않고 그대로 흘려보내왔는데(댓글과 반대), 콜백을 생략하는 것이 그 동작을 그대로 표현한다.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results660 tests +13 660 ✅ +13 2m 12s ⏱️ +20s Results for commit 8ad4560. ± Comparison against base commit 90097d6. This pull request removes 2 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Coverage
|
theminjunchoi
left a comment
theminjunchoi
left a comment
There was a problem hiding this comment.
재시도 루프와 대기를 분리한 건 깔끔한 것 같아요. 특히 토큰 누산, 로그를 executor 로 안 가져간 판단에 동의합니다 - 가져갔으면 executor 가 GenerationType·recorder 까지 알아야 했을 것 같아요.
하나만 여쭤보면, backoffMillis: Long 이 고정 대기만 표현할 수 있어서 #162 에서 지수 백오프를 넣으려면 executor 를 다시 수정해야 할 것 같습니다.
RetrySleeper 처럼 BackoffPolicy (delayMillisFor(attempt): Long) 를 fun interface 로 받으면 #162 가 구현 추가만으로 끝날 것 같은데, 이번 PR 범위로 볼지 #162 에서 같이 할지 어떻게 생각하세요?
대기 시간을 숫자 하나로 받으면 고정 간격만 표현할 수 있어, #162 의 지수 백오프를 넣을 때 executor 를 다시 고쳐야 한다. BackoffPolicy 로 받아 구현 추가만으로 끝나게 한다. 시도 번호뿐 아니라 실패한 예외도 함께 넘긴다. #162 의 대기 규칙이 시도 번호가 아니라 실패 종류로 갈리기 때문이다 — 429 는 짧게 기다리면 그대로 또 429 라 고정 간격이 필요하고, 응답 검증 실패는 Gemini 가 멀쩡하니 기다릴 이유가 없다. 정책이 시도마다 다시 호출되는지, 그 시도를 실패시킨 예외가 그대로 전달되는지를 테스트로 고정했다. 둘 다 고정 간격 테스트만으로는 잡히지 않는데, 어긋나면 #162 의 지수 백오프와 429 분기가 조용히 죽는다. 기본값이 fixed(RETRY_BACKOFF_MILLIS)(=0) 이라 동작은 그대로다.
|
반영했습니다. 시그니처만 한 칸 넓혀서 정말 구현 추가만으로 끝나는지 미리 확인해봤습니다. #162 정책을 임시로 구현해보니 executor 를 한 줄도 안 고치고 지수 백오프 기본값이 |
2 participants
🔗 연관 이슈
📌 개요
실사용자 유입 전에 LLM 호출 실패 대비를 갖추려는 작업(#17) 의 첫 단계다. 백오프·서킷브레이커를 붙이려면
재시도 루프부터 손봐야 하는데, 지금 90줄짜리 거의 같은 루프가 댓글·답글에 두 벌 있고 카드까지 재시도를
붙이면 네 벌이 된다. 정책을 바꾸기 전에 루프를 한 곳으로 모았다.
이 PR 은 동작을 바꾸지 않는다. 시도 횟수(댓글·답글 2회, 카드 1회)도 백오프(0ms)도 그대로다.
재시도 정책은 #162, 서킷브레이커는 #163 에서 다룬다.
🔧 주요 변경사항
LlmRetryExecutor신설, 호출 지점 4곳 전환 (댓글 · 답글 · 감정 분류 · 카드 한 줄)GenerationLog기록은 executor 로 가져오지 않았다. 검증에 실패한 시도도 호출은됐으니 과금되는데, 선언적 재시도(
@Retryable등)는 시도 사이에 끼어들 자리가 없어 과소 집계를 막을 수없다. 대신
call에 시도 번호를 넘기고 실패 시점마다 콜백을 열어 호출자가 계속 담당한다BackoffPolicy, 실제로 기다리는 것은RetrySleeper.기본값이
fixed(RETRY_BACKOFF_MILLIS)(=0) 이라 아직 아무도 대기하지 않는다CARD_MAX_ATTEMPTS = 1로 현행 유지. 하드코딩돼 있던attemptCount = 1은 executor 가넘겨주는 실제 시도 번호로 바꿔, 시도를 늘리면 저절로 맞는 값이 남는다
TokenUsageAccumulator를llm/generation으로 옮겨 카드 경로도 쓰게 했다. 시도가 1회인 지금은 값이같지만, 누산기 없이 시도만 늘리면 마지막 시도의 토큰만 기록돼 과금이 과소 집계된다 — 댓글에서 이미 한 번
났던 사고라 구조를 먼저 맞춰뒀다
🌐 API · DB 영향
generation_log에null대신0이 남는 값 변화만 있다. 누산기를 거치며 생긴 유일한 차이이고,used_tokens를 읽는 세 곳이 모두 이미null 을 0 으로 접고 있어 집계 결과는 달라지지 않는다 —
GenerationLogRepository의coalesce(sum(...), 0),ConversationUsageService,QualityStatsService. 댓글 경로는 원래 0 을남기고 있어 두 경로가 오히려 같아진다)
💬 리뷰 포인트
1. 동작이 정말 같은가
이 PR 에서 봐야 할 것은 사실상 이것 하나다. 기존 테스트를 한 줄도 고치지 않고 통과하는 것을 기준으로
삼았다 — 테스트 파일 변경은 추가뿐이고 삭제는 0줄이다(
git diff --numstat로 확인 가능). 예외는TokenUsageAccumulatorTest하나인데, 클래스를 옮기면서package·import줄만 바뀌고 기존 테스트2개의 본문은 그대로다.
./gradlew test --rerun-tasks로 빌드 캐시를 무시하고 돌려 88 클래스 660 테스트가 통과하는 것을확인했다(
build jacocoTestReport도 통과).2. 콜백 3개가 적정한가
onAttemptFailure(토큰 누산) ·onNonRetryable(예상 못 한 예외) ·onExhausted(소진) 로 나눴다.셋으로 가른 이유는 댓글과 카드의 동작이 원래 다르기 때문이다 — 댓글은 예상 못 한 예외도 실패 로그를
남기고, 카드는 기록도 상태 되돌림도 없이 그대로 흘려보낸다. 카드가
onNonRetryable을 넘기지 않는 것이그 현행 동작을 그대로 표현한다.
onNonRetryable을 없애고 호출자가 executor 바깥에서try/catch하는 안도 있었는데, 그러면 몇 번째시도에서 터졌는지를 잃어버려
attemptCount가 지금과 달라진다.3.
BackoffPolicy의 시그니처 (리뷰 반영, 8ad4560)대기 시간을 숫자 하나로 받으면 고정 간격만 표현할 수 있어 #162 에서 executor 를 다시 고쳐야 한다는
지적을 받아 정책 인터페이스로 바꿨다. 시도 번호뿐 아니라 실패한 예외도 함께 받는데, #162 의 대기
규칙이 시도 번호가 아니라 실패 종류로 갈리기 때문이다 — 네트워크·5xx 는 0.5s→1s 지수, 429 는 짧게
기다려봐야 그대로 또 429 라 고정 간격, 응답 검증 실패는 Gemini 가 멀쩡하니 즉시.
정말 구현 추가만으로 끝나는지 #162 정책을 임시로 구현해 확인했다. executor 를 한 줄도 고치지 않고
지수 백오프
[500, 1000]과 429 고정[2500, 2500]이 그대로 나왔다.4. 새로 추가한 테스트 4개
onNonRetryable경로가 원래 테스트로 한 번도 지나가지 않고 있었다. 기존 "예상치 못한 예외" 테스트 두 개는재시도 루프 바깥인
saveFeed에서 예외를 던지고 있어서다. 리팩터링으로 이 처리가 콜백 한 줄이 되면서,빠뜨려도 컴파일과 기존 테스트가 모두 통과하는 상태가 됐다. 실제로 배선을 지워보고 확인했다 — 지워도
655개가 전부 통과한다. 그래서 댓글·답글 각각에 실패 로그가 남는지 검증하는 테스트를 더했다.
BackoffPolicy쪽도 같은 이유로 둘을 더했다. 정책이 시도마다 다시 호출되는지, 그 시도를 실패시킨 예외가그대로 전달되는지인데, 고정 간격 테스트만으로는 둘 다 잡히지 않는다(어느 시도에서 물어보든 같은 값이
나오므로). 어긋나면 #162 의 지수 백오프와 429 분기가 조용히 죽는다.
5. 카드 쪽 줄 수가 늘었다
maxAttempts = 1인데 executor 를 쓰니 당연히 길어진다. 지금 얻는 이득은 없고 #162 에서 상수 한 줄만바꾸면 되도록 만드는 것이 목적이다. 구조 이동과 정책 변경을 같은 diff 에 섞지 않으려고 나눴다.