Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the scraping callback mechanism to fetch result payloads from S3 using a provided key instead of receiving them directly in the request body. Key additions include the ScrapeResultResultStoreClient for S3 interaction, ScrapeResultCallbackTxService for transactional state management, and a new POST_PROCESSING status for ScrapeJob. Review feedback identifies a security risk where the request body is parsed before HMAC verification, a logic error in performing checksum validation on normalized rather than raw S3 data, and redundant error handling between the callback service and the post-processor.
hoooonshub
left a comment
hoooonshub
left a comment
There was a problem hiding this comment.
코드 보기가 조금 어려워서 일단 보이는데만 달았어.
코드 머지하며는 IDE로 봐볼게.
확실히 ai 코드는 읽기가 어렵다,,,ㅋㅋㅋ 머지해주면 나도 ai로 분석해야될 것 같아 ㅋㅋㅋㅋㅋ
이게 지금 요청부터 끝까지 다 동기 처리로 작업하는거지?
비동기로 처리하게 되며는 executor로 쓰지 말고 아예 SQS로 하나의 메시지로 넘겨버리는 방식도 있을 것 같은데 일단 고려해줘
| @RequiredArgsConstructor | ||
| public class ScrapeResultResultStoreClient { |
There was a problem hiding this comment.
이거 클래스 이름 'Result' 중복됐어. 의도된거야?
| ## Checklist | ||
| - [ ] Domain tests: job 상태 전이/멱등 로직 추가 | ||
| - [ ] Application layer: 콜백 서비스 DTO/상태 저장/후처리 트리거 구현 | ||
| - [ ] Infrastructure layer: S3 client + 어댑터 + 설정 추가 |
There was a problem hiding this comment.
s3 관련된 설정 클래스는 따로 없는데, 어떻게 설정돼?
| @Bean | ||
| public S3Client scrapeResultS3Client() { | ||
| ScrapingProperties.ResultStore store = scrapingProperties.getResultStore(); | ||
| return S3Client.builder() | ||
| .region(Region.of(store.getRegion())) | ||
| .overrideConfiguration(ClientOverrideConfiguration.builder() | ||
| .apiCallTimeout(java.time.Duration.ofSeconds(store.getApiCallTimeoutSeconds())) | ||
| .apiCallAttemptTimeout(java.time.Duration.ofSeconds(store.getApiCallAttemptTimeoutSeconds())) | ||
| .build()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
아 s3 관련 파일 이거구나, 요거랑 ScrapingProperties,
이거 아예 s3 config로 빼버리는게 더 직관적일 것 같아.
|
@hoooonshub AI로 하다보니 놓친 이슈나 실수가 많았네요.. 더 철저하게 더블 체크하겠습니다 |
2 participants
변경 사항
/internal/scrape-results의 긴 단일 트랜잭션 구조를 분리했습니다.scrape_jobs상태에POST_PROCESSING을 추가해 worker 실행과 backend 후처리를 구분했습니다.RUNNING전이, stale timeout 판정 기준도 함께 정리했습니다.참고 사항
P1: 꼭 반영해주세요 (Request changes)
P2: 적극적으로 고려해주세요 (Request changes)
P3: 웬만하면 반영해 주세요 (Comment)
P4: 반영해도 좋고 넘어가도 좋습니다 (Approve)
P5: 그냥 사소한 의견입니다 (Approve)
🔗 Related Issue
Closes #215