[REFAC#393] validate worker + cmd/processor Kafka I/O → publisher facade - #408
Conversation
… (이슈 #393) 메타 #385 Sub 8 — validate worker 의 Kafka I/O 책임을 publisher facade 로 위임. 동일 패턴을 cmd/issuetracker + cmd/processor 양쪽 entry point 에 wiring 일관 적용. ## validate worker - 필드 `consumer queue.Consumer` → `consumer publisher.Consumer` (별칭) - 필드 `producer queue.Producer` → `pub *publisher.Publisher` - 4 종 직접 publish 모두 `w.pub.Forward(...)` 위임: - TopicValidated publish (검증 통과 → downstream) - TopicDLQ publish (검증 실패 라우팅) - TopicNormalized publish (재큐잉) - reparse publish (validator → parser 재학습 trigger, #366) - NewWorker 시그니처: consumer + producer → consumer + pub ## cmd/issuetracker/main.go - `validateProducer` 직접 주입 → `publisher.New(validateProducer, nil, log)` 로 thin wrap - validate 는 resolver / guard 불필요 — Forward 전용이라 nil resolver 로 충분 - crawlerProducer 와는 다른 producer 유지 (validateKafkaCfg.GroupID = GroupValidators 분리) ## cmd/processor/main.go (validator 단독 entry) - 동일 패턴: `publisher.New(producer, nil, log)` → validate.NewWorker 에 pub 전달 ## 테스트 - `test/.../validate/worker_test.go` `newTestPublisher(producer)` 헬퍼 도입 (Sub 5/7 동일 패턴) - `newWorker` 헬퍼 + 4 개 reparse 테스트 모두 `newTestPublisher(producer)` 래핑 - 검증 로직 / DLQ 분기 / reparse trigger 무변경 — pub.Forward 가 producer.Publish 위임이라 기존 mockProducer expectations 그대로 작동 ## 검증 - `go build ./internal/... ./cmd/issuetracker/ ./cmd/processor/ ./test/...` — pass - `go test -race -count=1 -timeout=180s ./test/internal/processor/validate/... + 관련` — 전 패키지 통과 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors the validate worker to accept a publisher facade instead of a raw Kafka producer. The worker's constructor and struct are updated to take ChangesPublisher Facade Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/processor/validate/worker.go (1)
61-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
pubis nil inNewWorker
pubis now a pointer dependency (Line 63) and is dereferenced in multiple paths; if a caller passes nil, processing panics at runtime. Add a constructor guard so failure is immediate and explicit.Suggested fix
func NewWorker( consumer publisher.Consumer, pub *publisher.Publisher, contentSvc service.ContentService, gate locks.StageGate, workerCount int, cfg config.ValidateConfig, ) *Worker { + if pub == nil { + panic("validate.NewWorker: pub must not be nil") + } if gate == nil { gate = locks.NewNoopStageGate() } return &Worker{ consumer: consumer, pub: pub,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/processor/validate/worker.go` around lines 61 - 75, NewWorker currently accepts pub *publisher.Publisher and dereferences it later; add a fail-fast guard at the start of NewWorker to explicitly reject a nil pub (similar to the existing gate nil handling) by checking if pub == nil and panicking with a clear message like "publisher.Publisher is nil in NewWorker" (or returning a wrapped error if you prefer to change the signature), so callers get an immediate, descriptive failure instead of a downstream panic when methods on pub are invoked; update the constructor body around NewWorker and the returned *Worker to ensure tests and callers expect this explicit failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/processor/validate/worker.go`:
- Around line 61-75: NewWorker currently accepts pub *publisher.Publisher and
dereferences it later; add a fail-fast guard at the start of NewWorker to
explicitly reject a nil pub (similar to the existing gate nil handling) by
checking if pub == nil and panicking with a clear message like
"publisher.Publisher is nil in NewWorker" (or returning a wrapped error if you
prefer to change the signature), so callers get an immediate, descriptive
failure instead of a downstream panic when methods on pub are invoked; update
the constructor body around NewWorker and the returned *Worker to ensure tests
and callers expect this explicit failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c799252-213f-4691-a055-bca480a62133
📒 Files selected for processing (5)
cmd/issuetracker/main.gocmd/processor/main.gointernal/processor/validate/worker.gotest/internal/processor/validate/reparse_test.gotest/internal/processor/validate/worker_test.go
…bbit Major) CodeRabbit 피드백: - pub 은 4 publish 사이트 (validated / dlq / requeue / reparse) 가 dereference 하는 hard dependency. nil 주입 시 publisher.Forward 가 error 를 반환하지만, validate worker 가 publish 실패 시 commit skip → Kafka 무한 재배달 → 운영 진단 어려움. - NewWorker 생성 시점에 panic 으로 fail-fast — silent failure 보다 안전. - 호출 site (cmd/issuetracker / cmd/processor / 테스트) 모두 이미 non-nil pub 주입 — 회귀 0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the validate.Worker to depend on the publisher.Publisher facade instead of the queue.Producer interface directly. This change decouples the worker from the underlying queue implementation and involves updating the Worker struct, its constructor, and all internal publishing calls to use the pub.Forward method. Entry points in cmd/issuetracker and cmd/processor have been updated to initialize the publisher facade, and test suites now utilize a newTestPublisher helper to wrap mock producers. I have no feedback to provide as no review comments were submitted.
연관 이슈
Closes #393
부모 메타: #385 — Publisher 통합 Sub 8 (마지막)
구현 내용
validate worker 의 Kafka I/O 책임을 publisher facade 로 위임. Sub 5 (#390) / Sub 7 (#392) 와 동일 패턴 — 같은 추상화 (publisher.Consumer / Forward) 를 마지막 stage 인 validate 에도 일관 적용.
validate worker 변경
consumer queue.Consumerconsumer publisher.Consumer(alias)producer queue.Producerpub *publisher.Publisherw.producer.Publish(...)w.pub.Forward(...)NewWorker(consumer, producer, ...)NewWorker(consumer, pub, ...)4 publish 사이트:
cmd/issuetracker/main.go wiring
validate 는 resolver / guard 불필요 (Forward 전용) —
publisher.New(producer, nil, log)로 thin wrap. crawlerProducer 와는 GroupID 가 다른 별개 producer 유지 (GroupValidators).cmd/processor/main.go (validator 단독 entry) wiring
테스트
test/internal/processor/validate/worker_test.gonewTestPublisher(producer)헬퍼 도입 (Sub 5/7 의 동일 패턴).newWorker헬퍼 + 4 개 reparse_test 케이스 모두newTestPublisher(producer)래핑.CI / 머지 게이트 점검
gofmt -l internal/ cmd/ test/— cleango build ./internal/... ./cmd/issuetracker/ ./cmd/processor/ ./test/...— passgo test -race -count=1 -timeout=180s ./test/internal/processor/validate/... ./test/internal/publisher/... + 관련— 전 패키지 통과[REFAC#393][REFAC]:prefix + 한국어변경 영향 범위 + 위험도
후속
Sub 8 머지 후 메타 #385 의 남은 sub:
Sub 6 까지 머지되면 메타 #385 close 가능.
롤백 계획
PR revert 시 모든 시그니처가 동시에 원복 — wiring 회귀 없음.
🤖 Generated with Claude Code
Summary by CodeRabbit