Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough트랜스코더가 DB 중심으로 재구성됨: Command 인터페이스 확장, CommandExtractor가 IngestJob/IngestCommand를 DB에서 조회·저장, 명령별 실행이 업로드 결과(String)를 반환하고 IngestJobStatusManager가 상태 전이를 관리하도록 변경됨. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JobOrchestrator
participant CommandExtractor
participant Repo as DB/Repos
participant Pipeline as CommandPipeline
participant VideoStorage
participant StatusMgr as IngestJobStatusManager
Client->>JobOrchestrator: handle(TranscodeMessage)
JobOrchestrator->>StatusMgr: startProcessing(ingestJobId)
JobOrchestrator->>CommandExtractor: extractCommand(msg, probe)
CommandExtractor->>Repo: findIngestJob / findCompletedKeys / save new IngestCommand(PENDING)
CommandExtractor-->>JobOrchestrator: List<Command> (pending)
loop per Command
JobOrchestrator->>Pipeline: execute(command, JobContext)
Pipeline->>VideoStorage: putFile(outputFile, destination)
VideoStorage-->>Pipeline: outputUrl
Pipeline-->>JobOrchestrator: outputUrl
alt command.type == TRANSCODE
JobOrchestrator->>StatusMgr: completeTranscodeCommand(ingestJobId, command, outputUrl)
else
JobOrchestrator->>StatusMgr: completeCommand(ingestJobId, command, outputUrl)
end
end
JobOrchestrator->>StatusMgr: checkAllCompleted(ingestJobId)
StatusMgr->>Repo: update IngestJob / Media 상태
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
modules/domain/src/main/java/com/ott/domain/short_form/repository/ShortFormRepository.java (1)
10-10:findByMediaId메서드 명명 패턴 검토메서드는 Spring Data JPA 파생 쿼리로 정상 작동합니다.
Media엔티티의id필드와ShortForm의@OneToOne관계를 통해 올바르게 해석됩니다. 다만 코드베이스에서는 단순 속성 조회(예:findByMediaId)에는 언더스코어 표기법을 사용하지 않고, 복합 조건 쿼리에서만 명시적 표기(findByMediaIdAndStatusAndMedia_PublicStatus)를 활용하는 패턴입니다. 현재 명명은 일관성 있으므로 그대로 유지해도 무방합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/domain/src/main/java/com/ott/domain/short_form/repository/ShortFormRepository.java` at line 10, The method findByMediaId in ShortFormRepository is consistent with the project's naming conventions for simple property lookups and can be left as-is; do not change the method name—leave Optional<ShortForm> findByMediaId(Long mediaId) in the ShortFormRepository interface untouched to maintain consistency with other simple queries.apps/transcoder/src/main/java/com/ott/transcoder/storage/VideoStorage.java (1)
27-32: 반환 타입 일관성 검토
upload메서드는String(업로드된 경로)을 반환하지만putFile은void를 반환합니다. 단일 파일의 목적지가 이미 알려져 있어 의도적일 수 있으나, 일관성 및 향후 확장성(예: 실제 URL 반환)을 위해 반환 타입 통일을 고려해 볼 수 있습니다.현재 호출 측에서 반환값이 필요 없다면 그대로 유지해도 무방합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/storage/VideoStorage.java` around lines 27 - 32, The VideoStorage interface has inconsistent return types: upload(Path,String) returns String while putFile(Path,String) is void; to make behavior consistent and allow future returns (e.g., stored URL), change putFile to return String (or an Optional<String>) and update implementations of VideoStorage and all callers to accept and propagate the returned storage path/value; alternatively, if callers truly don't need a return, document the void behavior clearly in the putFile Javadoc and leave as-is—refer to the methods upload and putFile in VideoStorage to locate the change.apps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/TranscodeProfile.java (1)
3-3: 동일 패키지 내 불필요한 import
Resolution이 동일 패키지(com.ott.transcoder.ffmpeg)에 있으므로 이 import는 불필요합니다. Java에서는 같은 패키지 내 클래스는 자동으로 접근 가능합니다.♻️ 제안된 수정
package com.ott.transcoder.ffmpeg; -import com.ott.transcoder.ffmpeg.Resolution; - /**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/TranscodeProfile.java` at line 3, Import com.ott.transcoder.ffmpeg.Resolution in TranscodeProfile is unnecessary because Resolution is in the same package; remove the unused import statement (the line importing Resolution) from the TranscodeProfile file and optionally run your IDE's organize/imports on the TranscodeProfile class to clean up any other redundant imports.apps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/Resolution.java (1)
22-27: 코드는 정상 운영 중 안전하며, 추가 예외 처리는 선택 사항입니다
Resolution.fromKey()의 호출 경로를 추적한 결과:
commandKey값은Resolution.values()에서만 생성되어 항상 유효한 열거형 상수("360P", "720P", "1080P")IngestJobStatusManager.getCompletedResolutions()는CommandType.TRANSCODE인 레코드만 필터링하므로, 유효하지 않은commandKey가 전달될 수 없음- 데이터 정상성이 보장되는 한
IllegalArgumentException은 발생하지 않음다만, 데이터베이스 손상/레거시 데이터로 인한 예외 상황에 대비하고 싶다면, 호출 측에서
try-catch로 명시적 예외 처리를 추가하는 것도 좋은 방어적 코딩 방식입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/Resolution.java` around lines 22 - 27, Resolution.fromKey(...) can throw IllegalArgumentException if an unexpected commandKey appears (e.g., DB corruption); wrap calls to Resolution.fromKey in a try-catch at the call site (e.g., inside IngestJobStatusManager.getCompletedResolutions where commandKey is used) and handle invalid keys by logging the error (include the bad key) and skipping or using a safe default instead of letting the exception propagate; ensure the catch targets IllegalArgumentException so only this case is handled and preserve normal flow for valid CommandType.TRANSCODE entries.modules/domain/src/main/java/com/ott/domain/ingest_job/domain/IngestJob.java (1)
41-44: 상태 전이 검증 고려현재
updateIngestStatus는 모든 상태 전이를 허용합니다. 잘못된 전이(예:SUCCESS→PENDING)를 방지하려면 도메인 레벨 검증 추가를 고려해볼 수 있습니다.상태 전이 로직이
IngestJobStatusManager에서 관리된다면 현재 구현도 괜찮습니다.♻️ 선택적: 도메인 레벨 상태 전이 검증 예시
public void updateIngestStatus(IngestStatus ingestStatus) { + if (this.ingestStatus == IngestStatus.SUCCESS || this.ingestStatus == IngestStatus.FAILED) { + throw new IllegalStateException("종료 상태에서는 상태 변경 불가: " + this.ingestStatus); + } this.ingestStatus = ingestStatus; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/domain/src/main/java/com/ott/domain/ingest_job/domain/IngestJob.java` around lines 41 - 44, The updateIngestStatus method currently allows any transition; add domain-level validation inside IngestJob.updateIngestStatus to prevent invalid transitions by checking the current this.ingestStatus against the target IngestStatus and either (a) validate via a centralized helper (e.g., IngestJobStatusManager.isValidTransition(current, newStatus)) or (b) implement an allowedTransitions map/enum check in the IngestStatus type, and throw a domain exception (e.g., IllegalStateException or a custom DomainException) when the transition is invalid; ensure callers that rely on external manager behavior still work by delegating to IngestJobStatusManager if present.modules/domain/src/main/java/com/ott/domain/media/domain/MediaStatus.java (1)
13-14: Enum 필드 접근 제어자 명시 권장
key와value필드가 기본 접근 제어자(package-private)로 선언되어 있습니다. 불변성을 보장하기 위해private final로 명시하는 것이 좋습니다.♻️ 필드 접근 제어자 수정 제안
- String key; - String value; + private final String key; + private final String value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/domain/src/main/java/com/ott/domain/media/domain/MediaStatus.java` around lines 13 - 14, The enum MediaStatus declares fields key and value with package-private visibility; change them to private final to enforce immutability and encapsulation in the MediaStatus enum, update the enum constructors to assign these fields (they likely already do) and ensure any external access uses the existing accessor methods (e.g., getKey/getValue) or add them if missing so callers don't rely on package-private field access.modules/infra-db/src/main/resources/db/migration/V9__transcode_status.sql (2)
29-49:ingest_command테이블에 복합 인덱스 추가 권장
IngestCommandRepository.findByIngestJobIdAndCommandKey()쿼리 패턴을 고려할 때,(ingest_job_id, command_key)복합 인덱스가 없으면 데이터 증가 시 조회 성능이 저하될 수 있습니다.📊 인덱스 추가 제안
ALTER TABLE ingest_command ADD CONSTRAINT fk_ingest_command_to_ingest_job FOREIGN KEY (ingest_job_id) REFERENCES ingest_job (id); + +-- 커맨드 조회 최적화를 위한 복합 인덱스 +CREATE INDEX idx_ingest_command_job_key + ON ingest_command (ingest_job_id, command_key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-db/src/main/resources/db/migration/V9__transcode_status.sql` around lines 29 - 49, Add a composite index on (ingest_job_id, command_key) to the ingest_command table to optimize queries used by IngestCommandRepository.findByIngestJobIdAndCommandKey(); modify the migration that creates ingest_command (the CREATE TABLE / ALTER TABLE block shown) to include an index such as idx_ingest_job_id_command_key using ALTER TABLE ... ADD INDEX (ingest_job_id, command_key) so lookups by ingest_job_id+command_key are covered and will scale as data grows.
16-22: SQL_SAFE_UPDATES 비활성화 구간 최소화
SQL_SAFE_UPDATES = 0설정이 전역적으로 적용되어 있습니다. 프로덕션 환경에서의 안전을 위해 트랜잭션 내에서 명시적으로 관리하거나, WHERE 절에 기본 키 조건을 추가하는 것이 더 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-db/src/main/resources/db/migration/V9__transcode_status.sql` around lines 16 - 22, The migration currently disables SQL_SAFE_UPDATES globally; instead, restrict that change to the minimal scope by wrapping the UPDATE in an explicit transaction and only toggling SQL_SAFE_UPDATES immediately before and after the single UPDATE statement, or better—avoid toggling entirely by adding a WHERE clause that targets rows via the primary key (e.g., UPDATE ingest_job SET ingest_status='PENDING' WHERE id IS NOT NULL OR a specific PK condition) so the UPDATE ingest_job in V9__transcode_status.sql does not rely on global SQL_SAFE_UPDATES changes.apps/transcoder/src/main/java/com/ott/transcoder/pipeline/CommandPipeline.java (1)
6-16: Javadoc@return태그 위치 수정 권장
@return태그가 인터페이스 레벨에 있지만,execute메서드에 직접 배치하는 것이 표준 Javadoc 규칙에 부합합니다.📝 Javadoc 위치 수정 제안
/** * 커맨드별 미디어 처리 파이프라인 * 구현체는 미디어 처리 + 산출물 업로드까지 담당 - * - * `@return` 업로드된 산출물의 S3 key (outputUrl) */ public interface CommandPipeline<T extends Command> { boolean support(Command command); + /** + * 커맨드를 실행하고 산출물을 업로드합니다. + * + * `@param` command 실행할 커맨드 + * `@param` jobContext 작업 컨텍스트 + * `@return` 업로드된 산출물의 S3 key (outputUrl) + */ String execute(T command, JobContext jobContext); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/pipeline/CommandPipeline.java` around lines 6 - 16, Move the misplaced Javadoc `@return` tag from the CommandPipeline interface-level comment into the execute method's Javadoc: remove the `@return` from the interface Javadoc and add a proper `@return` description to the execute(T command, JobContext jobContext) method comment explaining that it returns the uploaded artifact's S3 key (outputUrl); keep the interface-level description for the pipeline responsibility but ensure `@return` is only on the execute method.apps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitDeadLetterListener.java (1)
21-28:statusManager.fail()호출 시 예외 처리 고려
statusManager.fail()호출이 실패할 경우(예: DB 연결 오류), 예외가 전파되어 메시지가 다시 DLQ에 들어가거나 처리되지 않은 상태로 남을 수 있습니다. 또한, 실패 처리 후log.info()보다log.warn()또는 에러 문맥을 유지하는 것이 로그 추적에 더 적합할 수 있습니다.🛡️ 예외 처리 및 로그 레벨 개선 제안
`@RabbitListener`(queues = RabbitConfig.DEAD_LETTER_QUEUE) public void handleDeadLetter(TranscodeMessage message) { log.error("DLQ 수신 - mediaId: {}, ingestJobId: {}", message.mediaId(), message.ingestJobId()); - statusManager.fail(message.ingestJobId()); - - log.info("실패 처리 완료 - ingestJobId: {}", message.ingestJobId()); + try { + statusManager.fail(message.ingestJobId()); + log.warn("실패 처리 완료 - ingestJobId: {}", message.ingestJobId()); + } catch (Exception e) { + log.error("실패 상태 업데이트 중 오류 발생 - ingestJobId: {}", + message.ingestJobId(), e); + throw e; // 재처리 또는 모니터링을 위해 재전파 + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitDeadLetterListener.java` around lines 21 - 28, The handleDeadLetter method currently calls statusManager.fail(message.ingestJobId()) without protecting against exceptions; wrap that call in a try-catch, catch Throwable (or appropriate checked exceptions from statusManager.fail), log the failure with log.error including the exception and context (mediaId and ingestJobId), and on successful failure handling replace the final log.info with log.warn or log.error to preserve error context; ensure the catch block does not rethrow so the DLQ handler completes and consider invoking any cleanup/compensation logic if provided by statusManager.apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java (1)
40-56: 두 번의 DB 쿼리를 하나로 합칠 수 있습니다.
findByIngestJobIdAndCommandStatus(완료된 커맨드)와findByIngestJobId(전체 커맨드)를 각각 호출하고 있습니다. 전체 커맨드 조회 후 메모리에서 필터링하면 쿼리 하나로 줄일 수 있습니다.♻️ 단일 쿼리로 최적화
- // 2. 완료된 commandKey 조회 - Set<String> completedKeySet = ingestCommandRepository // key는 360P, 720P, thumbnail 등 고유 - .findByIngestJobIdAndCommandStatus(ingestJobId, CommandStatus.COMPLETED) - .stream() - .map(IngestCommand::getCommandKey) - .collect(Collectors.toSet()); - - // 3. 완료 제외 = 실행 대상 - List<Command> pendingList = candidateList.stream() - .filter(cmd -> !completedKeySet.contains(cmd.getCommandKey())) - .toList(); - - // 4. DB에 없는 것만 저장 - Set<String> existingKeySet = ingestCommandRepository - .findByIngestJobId(ingestJobId).stream() - .map(IngestCommand::getCommandKey) - .collect(Collectors.toSet()); + // 2. 기존 커맨드 조회 (한 번의 쿼리) + List<IngestCommand> existingCommandList = ingestCommandRepository.findByIngestJobId(ingestJobId); + + Set<String> completedKeySet = existingCommandList.stream() + .filter(ic -> ic.getCommandStatus() == CommandStatus.COMPLETED) + .map(IngestCommand::getCommandKey) + .collect(Collectors.toSet()); + + Set<String> existingKeySet = existingCommandList.stream() + .map(IngestCommand::getCommandKey) + .collect(Collectors.toSet()); + + // 3. 완료 제외 = 실행 대상 + List<Command> pendingList = candidateList.stream() + .filter(cmd -> !completedKeySet.contains(cmd.getCommandKey())) + .toList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java` around lines 40 - 56, Replace the two repository calls with a single call to ingestCommandRepository.findByIngestJobId(ingestJobId), then derive both completedKeySet and existingKeySet from that single result: build completedKeySet by filtering the fetched IngestCommand list for CommandStatus.COMPLETED (using IngestCommand::getCommandKey) and build existingKeySet from all fetched items (using IngestCommand::getCommandKey); keep the candidateList -> pendingList filtering logic unchanged so pendingList excludes completedKeySet.apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java (2)
37-45: enum 비교 시==사용을 권장합니다.41행에서
.equals()를 사용하고 있으나, 71행에서는==를 사용하여 일관성이 없습니다. enum 비교에는==가 null-safe하고 더 명확합니다.♻️ 일관된 enum 비교
- if (ingestJob.getIngestStatus().equals(IngestStatus.PENDING)) { + if (ingestJob.getIngestStatus() == IngestStatus.PENDING) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java` around lines 37 - 45, Replace the enum comparison in startProcessing to use identity comparison (==) instead of .equals(): when you retrieve the IngestJob via findIngestJob and call ingestJob.getIngestStatus(), compare it to IngestStatus.PENDING using "==" and then call ingestJob.updateIngestStatus(IngestStatus.PROCESSING) as before; this makes the comparison null-safe and consistent with other code that uses "==" (references: startProcessing, findIngestJob, IngestJob.getIngestStatus, IngestStatus.PENDING, updateIngestStatus).
99-111: 상태 전이 전 현재 상태 검증을 고려해 주세요.현재 로직은 미완료 커맨드가 없으면 무조건 SUCCESS로 업데이트합니다. 만약 Job이 이미 FAILED 상태라면(DLQ 처리 후 재호출 등 edge case) 의도치 않게 FAILED→SUCCESS로 전이될 수 있습니다.
현재 호출 흐름상 발생 가능성은 낮지만, 방어적으로 현재 상태 검증을 추가하는 것을 권장합니다.
🛡️ 방어적 상태 검증 추가
if (!hasIncomplete) { IngestJob ingestJob = findIngestJob(ingestJobId); + if (ingestJob.getIngestStatus() != IngestStatus.PARTIAL_SUCCESS) { + log.warn("예상치 못한 상태에서 완료 확인 - ingestJobId: {}, status: {}", + ingestJobId, ingestJob.getIngestStatus()); + return; + } ingestJob.updateIngestStatus(IngestStatus.SUCCESS); log.info("모든 커맨드 완료 - ingestJobId: {}, PARTIAL_SUCCESS → SUCCESS", ingestJobId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java` around lines 99 - 111, The checkAllCompleted method unconditionally sets IngestStatus.SUCCESS when no incomplete commands remain; change it to read the current job (via findIngestJob(ingestJobId)) and verify its existing status before calling ingestJob.updateIngestStatus(IngestStatus.SUCCESS) (e.g., only transition if current status is IN_PROGRESS or PARTIAL_SUCCESS), so you avoid flipping FAILED→SUCCESS in edge cases detected by ingestCommandRepository.existsByIngestJobIdAndCommandStatusNot; implement the conditional guard around updateIngestStatus to only perform the transition when the current state allows it.apps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.java (1)
63-64:startProcessing호출 위치가 try 블록 외부에 있어 cleanup이 누락될 수 있습니다.
statusManager.startProcessing(ingestJobId)가 try 블록 이전에 호출됩니다. 만약 65-66행 사이(createWorkDir 호출 전)에서 예외가 발생하면 finally 블록의 cleanup이 실행되지만, startProcessing이 실패하면 cleanup 없이 예외가 전파됩니다.startProcessing 호출을 try 블록 내부로 이동하거나, 현재 위치가 의도적인 경우 주석으로 명시하는 것을 권장합니다.
♻️ try 블록 내부로 이동 제안
Path workDir = Path.of(tempDir, PREFIX_WORK_DIR + mediaId + SUFFIX_WORK_DIR + ingestJobId); - // CP-3: 작업 시작 - statusManager.startProcessing(ingestJobId); try { + // CP-3: 작업 시작 + statusManager.startProcessing(ingestJobId); + // 1. 작업 디렉토리 생성 (공간 체크를 위해 디렉토리가 존재해야 함) createWorkDir(workDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.java` around lines 63 - 64, Move the statusManager.startProcessing(ingestJobId) call into the try block in JobOrchestrator (before createWorkDir is called) so that any exceptions thrown after starting processing still run the finally cleanup; if the current placement is intentional, add a clear inline comment near statusManager.startProcessing explaining why it must remain outside the try/finally to avoid accidental removal. Ensure references: statusManager.startProcessing, createWorkDir, and the finally cleanup block are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java`:
- Around line 113-123: The fail(Long ingestJobId) path called from the DLQ
listener must not throw when the job is missing: wrap the call to
findIngestJob(ingestJobId) (currently inside fail) in defensive handling that
catches the not-found/BusinessException case, log a warning (including
ingestJobId and exception message) and return without rethrowing so the DLQ
processing won’t loop; keep the existing behavior for existing IngestJob/Media
(updateIngestStatus and updateMediaStatus) unchanged and only suppress
exceptions caused by a missing job (do not swallow unrelated exceptions).
In
`@apps/transcoder/src/main/java/com/ott/transcoder/pipeline/hls/TranscodeCommandPipeline.java`:
- Around line 46-49: FFmpeg creates lowercase resolution folders but
TranscodeCommandPipeline currently uses commandKey as-is; update the path
construction to use commandKey.toLowerCase() when building the resolution
directory and ensure the upload prefix matches (i.e., use
jobContext.outputDir().resolve(commandKey.toLowerCase()) and use
commandKey.toLowerCase() when constructing destinationPrefix) so
videoStorage.upload(resolutionDir, destinationPrefix) points at the actual
FFmpeg output.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/queue/TranscodeMessage.java`:
- Around line 14-21: The TranscodeMessage record was extended with a new field
mediaType which will deserialize to null for older queue messages and cause NPEs
in JobOrchestrator.resolveUploadPrefix(); change the record signature to use
Optional<MediaType> (i.e. Optional<MediaType> mediaType) or provide a non-null
default via an additional canonical constructor/factory so deserialized
instances never expose a null mediaType, update any call sites that read
mediaType to handle Optional (or the default), and also enable Jackson's
DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES in your ObjectMapper
configuration to fail fast on missing creator properties during deployment if
you prefer strict compatibility.
In `@modules/domain/src/main/java/com/ott/domain/media/domain/Media.java`:
- Around line 68-74: The mediaStatus field can be null when using Lombok's
`@Builder` because it lacks a default; add Lombok's `@Builder.Default` to the
mediaStatus field and initialize it to a non-null default value (e.g.,
MediaStatus.PENDING or your domain-appropriate default) so
Media.builder()...build() never produces null; keep the
updateMediaStatus(MediaStatus) method unchanged so runtime updates still work.
---
Nitpick comments:
In
`@apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java`:
- Around line 40-56: Replace the two repository calls with a single call to
ingestCommandRepository.findByIngestJobId(ingestJobId), then derive both
completedKeySet and existingKeySet from that single result: build
completedKeySet by filtering the fetched IngestCommand list for
CommandStatus.COMPLETED (using IngestCommand::getCommandKey) and build
existingKeySet from all fetched items (using IngestCommand::getCommandKey); keep
the candidateList -> pendingList filtering logic unchanged so pendingList
excludes completedKeySet.
In `@apps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/Resolution.java`:
- Around line 22-27: Resolution.fromKey(...) can throw IllegalArgumentException
if an unexpected commandKey appears (e.g., DB corruption); wrap calls to
Resolution.fromKey in a try-catch at the call site (e.g., inside
IngestJobStatusManager.getCompletedResolutions where commandKey is used) and
handle invalid keys by logging the error (include the bad key) and skipping or
using a safe default instead of letting the exception propagate; ensure the
catch targets IllegalArgumentException so only this case is handled and preserve
normal flow for valid CommandType.TRANSCODE entries.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/TranscodeProfile.java`:
- Line 3: Import com.ott.transcoder.ffmpeg.Resolution in TranscodeProfile is
unnecessary because Resolution is in the same package; remove the unused import
statement (the line importing Resolution) from the TranscodeProfile file and
optionally run your IDE's organize/imports on the TranscodeProfile class to
clean up any other redundant imports.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java`:
- Around line 37-45: Replace the enum comparison in startProcessing to use
identity comparison (==) instead of .equals(): when you retrieve the IngestJob
via findIngestJob and call ingestJob.getIngestStatus(), compare it to
IngestStatus.PENDING using "==" and then call
ingestJob.updateIngestStatus(IngestStatus.PROCESSING) as before; this makes the
comparison null-safe and consistent with other code that uses "==" (references:
startProcessing, findIngestJob, IngestJob.getIngestStatus, IngestStatus.PENDING,
updateIngestStatus).
- Around line 99-111: The checkAllCompleted method unconditionally sets
IngestStatus.SUCCESS when no incomplete commands remain; change it to read the
current job (via findIngestJob(ingestJobId)) and verify its existing status
before calling ingestJob.updateIngestStatus(IngestStatus.SUCCESS) (e.g., only
transition if current status is IN_PROGRESS or PARTIAL_SUCCESS), so you avoid
flipping FAILED→SUCCESS in edge cases detected by
ingestCommandRepository.existsByIngestJobIdAndCommandStatusNot; implement the
conditional guard around updateIngestStatus to only perform the transition when
the current state allows it.
In `@apps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.java`:
- Around line 63-64: Move the statusManager.startProcessing(ingestJobId) call
into the try block in JobOrchestrator (before createWorkDir is called) so that
any exceptions thrown after starting processing still run the finally cleanup;
if the current placement is intentional, add a clear inline comment near
statusManager.startProcessing explaining why it must remain outside the
try/finally to avoid accidental removal. Ensure references:
statusManager.startProcessing, createWorkDir, and the finally cleanup block are
adjusted accordingly.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/pipeline/CommandPipeline.java`:
- Around line 6-16: Move the misplaced Javadoc `@return` tag from the
CommandPipeline interface-level comment into the execute method's Javadoc:
remove the `@return` from the interface Javadoc and add a proper `@return`
description to the execute(T command, JobContext jobContext) method comment
explaining that it returns the uploaded artifact's S3 key (outputUrl); keep the
interface-level description for the pipeline responsibility but ensure `@return`
is only on the execute method.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitDeadLetterListener.java`:
- Around line 21-28: The handleDeadLetter method currently calls
statusManager.fail(message.ingestJobId()) without protecting against exceptions;
wrap that call in a try-catch, catch Throwable (or appropriate checked
exceptions from statusManager.fail), log the failure with log.error including
the exception and context (mediaId and ingestJobId), and on successful failure
handling replace the final log.info with log.warn or log.error to preserve error
context; ensure the catch block does not rethrow so the DLQ handler completes
and consider invoking any cleanup/compensation logic if provided by
statusManager.
In `@apps/transcoder/src/main/java/com/ott/transcoder/storage/VideoStorage.java`:
- Around line 27-32: The VideoStorage interface has inconsistent return types:
upload(Path,String) returns String while putFile(Path,String) is void; to make
behavior consistent and allow future returns (e.g., stored URL), change putFile
to return String (or an Optional<String>) and update implementations of
VideoStorage and all callers to accept and propagate the returned storage
path/value; alternatively, if callers truly don't need a return, document the
void behavior clearly in the putFile Javadoc and leave as-is—refer to the
methods upload and putFile in VideoStorage to locate the change.
In
`@modules/domain/src/main/java/com/ott/domain/ingest_job/domain/IngestJob.java`:
- Around line 41-44: The updateIngestStatus method currently allows any
transition; add domain-level validation inside IngestJob.updateIngestStatus to
prevent invalid transitions by checking the current this.ingestStatus against
the target IngestStatus and either (a) validate via a centralized helper (e.g.,
IngestJobStatusManager.isValidTransition(current, newStatus)) or (b) implement
an allowedTransitions map/enum check in the IngestStatus type, and throw a
domain exception (e.g., IllegalStateException or a custom DomainException) when
the transition is invalid; ensure callers that rely on external manager behavior
still work by delegating to IngestJobStatusManager if present.
In `@modules/domain/src/main/java/com/ott/domain/media/domain/MediaStatus.java`:
- Around line 13-14: The enum MediaStatus declares fields key and value with
package-private visibility; change them to private final to enforce immutability
and encapsulation in the MediaStatus enum, update the enum constructors to
assign these fields (they likely already do) and ensure any external access uses
the existing accessor methods (e.g., getKey/getValue) or add them if missing so
callers don't rely on package-private field access.
In
`@modules/domain/src/main/java/com/ott/domain/short_form/repository/ShortFormRepository.java`:
- Line 10: The method findByMediaId in ShortFormRepository is consistent with
the project's naming conventions for simple property lookups and can be left
as-is; do not change the method name—leave Optional<ShortForm>
findByMediaId(Long mediaId) in the ShortFormRepository interface untouched to
maintain consistency with other simple queries.
In `@modules/infra-db/src/main/resources/db/migration/V9__transcode_status.sql`:
- Around line 29-49: Add a composite index on (ingest_job_id, command_key) to
the ingest_command table to optimize queries used by
IngestCommandRepository.findByIngestJobIdAndCommandKey(); modify the migration
that creates ingest_command (the CREATE TABLE / ALTER TABLE block shown) to
include an index such as idx_ingest_job_id_command_key using ALTER TABLE ... ADD
INDEX (ingest_job_id, command_key) so lookups by ingest_job_id+command_key are
covered and will scale as data grows.
- Around line 16-22: The migration currently disables SQL_SAFE_UPDATES globally;
instead, restrict that change to the minimal scope by wrapping the UPDATE in an
explicit transaction and only toggling SQL_SAFE_UPDATES immediately before and
after the single UPDATE statement, or better—avoid toggling entirely by adding a
WHERE clause that targets rows via the primary key (e.g., UPDATE ingest_job SET
ingest_status='PENDING' WHERE id IS NOT NULL OR a specific PK condition) so the
UPDATE ingest_job in V9__transcode_status.sql does not rely on global
SQL_SAFE_UPDATES changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80482cb9-8953-4d53-b777-b192734c75d8
📒 Files selected for processing (30)
apps/transcoder/src/main/java/com/ott/transcoder/command/Command.javaapps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.javaapps/transcoder/src/main/java/com/ott/transcoder/command/TranscodeCommand.javaapps/transcoder/src/main/java/com/ott/transcoder/config/TranscodeErrorHandler.javaapps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/Resolution.javaapps/transcoder/src/main/java/com/ott/transcoder/ffmpeg/TranscodeProfile.javaapps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.javaapps/transcoder/src/main/java/com/ott/transcoder/job/JobContext.javaapps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.javaapps/transcoder/src/main/java/com/ott/transcoder/pipeline/CommandPipeline.javaapps/transcoder/src/main/java/com/ott/transcoder/pipeline/CommandPipelineExecutor.javaapps/transcoder/src/main/java/com/ott/transcoder/pipeline/hls/MasterPlaylistGenerator.javaapps/transcoder/src/main/java/com/ott/transcoder/pipeline/hls/TranscodeCommandPipeline.javaapps/transcoder/src/main/java/com/ott/transcoder/pipeline/hls/TranscodePlanner.javaapps/transcoder/src/main/java/com/ott/transcoder/queue/TranscodeMessage.javaapps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitDeadLetterListener.javaapps/transcoder/src/main/java/com/ott/transcoder/storage/LocalVideoStorage.javaapps/transcoder/src/main/java/com/ott/transcoder/storage/S3VideoStorage.javaapps/transcoder/src/main/java/com/ott/transcoder/storage/VideoStorage.javamodules/common-web/src/main/java/com/ott/common/web/exception/ErrorCode.javamodules/domain/src/main/java/com/ott/domain/ingest_command/domain/CommandStatus.javamodules/domain/src/main/java/com/ott/domain/ingest_command/domain/CommandType.javamodules/domain/src/main/java/com/ott/domain/ingest_command/domain/IngestCommand.javamodules/domain/src/main/java/com/ott/domain/ingest_command/repository/IngestCommandRepository.javamodules/domain/src/main/java/com/ott/domain/ingest_job/domain/IngestJob.javamodules/domain/src/main/java/com/ott/domain/ingest_job/domain/IngestStatus.javamodules/domain/src/main/java/com/ott/domain/media/domain/Media.javamodules/domain/src/main/java/com/ott/domain/media/domain/MediaStatus.javamodules/domain/src/main/java/com/ott/domain/short_form/repository/ShortFormRepository.javamodules/infra-db/src/main/resources/db/migration/V9__transcode_status.sql
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java (1)
61-63: 컬렉션 파라미터명은List접미사로 맞춰 주세요.
parts보다partETagList처럼 컬렉션임이 드러나는 이름이 가이드와 더 일관됩니다.As per coding guidelines, "Collection variable names with
Listsuffix".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` around lines 61 - 63, Rename the collection parameter in completeContentsOriginUpload from parts to partETagList to follow the "Collection variable names with List suffix" guideline; update the method signature in BackOfficeContentsService.completeContentsOriginUpload, adjust all internal references inside that method from parts to partETagList, and update all call sites that pass or reference that parameter to use the new name (ensure any unit tests or overrides/implementations are updated accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsReader.java`:
- Around line 70-80: The tag lookup uses originMediaId (overridden to the
series' media id) which causes series-level tags to be fetched instead of the
current content's media tags; in BackOfficeContentsReader update the logic so
mediaTagRepository.findWithTagAndCategoryByMediaId(...) is called with the
current content's media id (e.g., contents.getMedia().getId() or the existing
mediaId variable) rather than originMediaId, while still keeping seriesTitle and
seriesId populated from contents.getSeries() when present; remove or stop
overriding originMediaId with originMedia.getId() and pass the content media id
to findWithTagAndCategoryByMediaId.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 71-78: The code creates an IngestJob via writer.createIngestJob
(returning IngestJobResult) but never publishes the transcode request, leaving
jobs in PENDING; restore the transcode publish by wiring the publisher and
emitting a TranscodeMessage after creating the job (use
transcodePublisher.publish(...) with result.mediaId(), result.ingestJobId(),
result.originObjectKey(), result.fileSize(), result.mediaType()), or implement
an outbox entry if async/durable delivery is required; update
BackOfficeContentsService.completeContentsOriginUpload to call the transcode
publish (or persist an outbox record) outside the write transaction so the
ingest flow proceeds.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsWriter.java`:
- Around line 142-144: The current check in BackOfficeContentsWriter uses
ingestJobRepository.existsByMediaId(media.getId()), which blocks reprocessing
for any previous terminal states (including FAILED or PARTIAL_SUCCESS); change
this to a status-aware repository query on IngestJobRepository (e.g.,
existsByMediaIdAndStatusIn or existsByMediaIdAndStatusNotIn) so you only throw
BusinessException(ErrorCode.ALREADY_INGESTED) when a non-reprocessable status
exists (for example only when there is an existing SUCCESS/COMPLETED ingest or
an active IN_PROGRESS as appropriate), and update the call in
BackOfficeContentsWriter to use that new method instead of existsByMediaId(...).
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 61-63: Rename the collection parameter in
completeContentsOriginUpload from parts to partETagList to follow the
"Collection variable names with List suffix" guideline; update the method
signature in BackOfficeContentsService.completeContentsOriginUpload, adjust all
internal references inside that method from parts to partETagList, and update
all call sites that pass or reference that parameter to use the new name (ensure
any unit tests or overrides/implementations are updated accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ceaa0f8-d7e2-4571-ad24-ade73a8155cf
📒 Files selected for processing (8)
apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsReader.javaapps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.javaapps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsWriter.javaapps/api-admin/src/main/java/com/ott/api_admin/content/vo/IngestJobResult.javaapps/api-admin/src/main/java/com/ott/api_admin/series/service/BackOfficeSeriesService.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javamodules/common-web/src/main/java/com/ott/common/web/exception/ErrorCode.javamodules/domain/src/main/java/com/ott/domain/ingest_job/repository/IngestJobRepository.java
Comment on lines
+70
to
+80
| Long originMediaId = mediaId; | ||
| String seriesTitle = null; | ||
| Long seriesId = null; | ||
| if (contents.getSeries() != null) { | ||
| Media originMedia = contents.getSeries().getMedia(); | ||
| originMediaId = originMedia.getId(); | ||
| seriesTitle = originMedia.getTitle(); | ||
| seriesId = contents.getSeries().getId(); | ||
| } | ||
|
|
||
| List<MediaTag> mediaTagList = mediaTagRepository.findWithTagAndCategoryByMediaId(originMediaId); |
There was a problem hiding this comment.
시리즈 콘텐츠 상세의 태그 조회 대상이 잘못되었습니다.
여기서는 시리즈의 mediaId로 태그를 조회하지만, 생성/수정 경로는 둘 다 현재 contents의 media에 태그를 저장합니다. 그래서 시리즈 소속 콘텐츠 상세에서 태그가 비어 보이거나 시리즈 태그로 잘못 보일 수 있습니다.
🔧 수정 예시
- Long originMediaId = mediaId;
String seriesTitle = null;
Long seriesId = null;
if (contents.getSeries() != null) {
Media originMedia = contents.getSeries().getMedia();
- originMediaId = originMedia.getId();
seriesTitle = originMedia.getTitle();
seriesId = contents.getSeries().getId();
}
- List<MediaTag> mediaTagList = mediaTagRepository.findWithTagAndCategoryByMediaId(originMediaId);
+ List<MediaTag> mediaTagList = mediaTagRepository.findWithTagAndCategoryByMediaId(media.getId());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsReader.java`
around lines 70 - 80, The tag lookup uses originMediaId (overridden to the
series' media id) which causes series-level tags to be fetched instead of the
current content's media tags; in BackOfficeContentsReader update the logic so
mediaTagRepository.findWithTagAndCategoryByMediaId(...) is called with the
current content's media id (e.g., contents.getMedia().getId() or the existing
mediaId variable) rather than originMediaId, while still keeping seriesTitle and
seriesId populated from contents.getSeries() when present; remove or stop
overriding originMediaId with originMedia.getId() and pass the content media id
to findWithTagAndCategoryByMediaId.
Comment on lines
+142
to
+144
| if (ingestJobRepository.existsByMediaId(media.getId())) { | ||
| throw new BusinessException(ErrorCode.ALREADY_INGESTED); | ||
| } |
There was a problem hiding this comment.
existsByMediaId()로는 terminal 상태 재처리를 막아버립니다.
제공된 IngestJobRepository 스니펫상 existsByMediaId(...) 는 mediaId만 검사합니다. 이대로면 이미 FAILED나 PARTIAL_SUCCESS로 끝난 이력도 모두 ALREADY_INGESTED로 막혀서 복구/재처리 경로가 사라집니다. 차단 기준을 status-aware query로 바꾸는 쪽이 맞습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsWriter.java`
around lines 142 - 144, The current check in BackOfficeContentsWriter uses
ingestJobRepository.existsByMediaId(media.getId()), which blocks reprocessing
for any previous terminal states (including FAILED or PARTIAL_SUCCESS); change
this to a status-aware repository query on IngestJobRepository (e.g.,
existsByMediaIdAndStatusIn or existsByMediaIdAndStatusNotIn) so you only throw
BusinessException(ErrorCode.ALREADY_INGESTED) when a non-reprocessable status
exists (for example only when there is an existing SUCCESS/COMPLETED ingest or
an active IN_PROGRESS as appropriate), and update the call in
BackOfficeContentsWriter to use that new method instead of existsByMediaId(...).
Contributor
Author
There was a problem hiding this comment.
업로드쪽이라서 이미 실패한 Ingest Job이 들어오진 않을 것이기에 우선 냅뒀습니다.
업로드 구현하신 분이 짜 둔 것이기도 하구요
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
modules/infra-mq/src/main/java/com/ott/infra/mq/TranscodeMessage.java (1)
14-20: 필수 필드에 대한 null 검증 고려
mediaId와ingestJobId는 트랜스코딩 처리에 필수적인 값으로 보입니다. 현재 record는 Jackson 역직렬화 시 null 값을 허용합니다. 잘못된 메시지로 인한 런타임 오류를 방지하려면 compact constructor에서 검증을 추가하는 것을 권장합니다.♻️ null 검증 추가 제안
public record TranscodeMessage( Long mediaId, Long ingestJobId, String originUrl, Long fileSize, MediaType mediaType ) { + public TranscodeMessage { + if (mediaId == null) { + throw new IllegalArgumentException("mediaId must not be null"); + } + if (ingestJobId == null) { + throw new IllegalArgumentException("ingestJobId must not be null"); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-mq/src/main/java/com/ott/infra/mq/TranscodeMessage.java` around lines 14 - 20, TranscodeMessage's record fields mediaId and ingestJobId must be non-null but are currently allowed to be null after Jackson deserialization; add a compact constructor in the TranscodeMessage record that validates mediaId and ingestJobId (e.g., via Objects.requireNonNull or explicit checks) and throws a clear exception (NullPointerException/IllegalArgumentException) with a descriptive message when they are null so invalid messages fail fast during deserialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java`:
- Around line 47-50: The pendingList calculation in CommandExtractor currently
only excludes commands whose keys are in completedKeySet, allowing commands with
status PROCESSING to be retried; update the filter that builds pendingList (from
candidateList.stream()) to also exclude commands in PROCESSING state (e.g.,
check cmd.getStatus() != CommandStatus.PROCESSING or maintain a processingKeySet
and exclude keys present there) so that commands already being handled by other
consumers are not returned for re-execution (also apply the same exclusion where
pendingList is returned).
- Around line 53-69: The current read-then-insert flow (existingKeySet built
from ingestCommandRepository.findByIngestJobId(...) then
ingestCommandRepository.saveAll(...)) is vulnerable to TOCTOU races causing
duplicate-key errors under concurrent consumers; change to a concurrency-safe
insert strategy: instead of relying solely on the pre-read set, perform a
DB-side upsert/insert-ignore or catch-and-ignore duplicate-key exceptions when
persisting the IngestCommand entities (the block building newCommandList from
pendingList into IngestCommand.builder(...) and calling
ingestCommandRepository.saveAll(...)). Implement one of: (a) a repository method
that executes a bulk "insert ... on conflict do nothing" / "insert ignore" using
a native query, or (b) keep saveAll but wrap each insert in try/catch and
swallow only the duplicate-key exception (SQLState/code), ensuring other
exceptions propagate; target methods/symbols: existingKeySet,
ingestCommandRepository.findByIngestJobId, newCommandList construction,
IngestCommand.builder(...), and ingestCommandRepository.saveAll(...) so the fix
replaces the vulnerable saveAll path with DB-level upsert or duplicate-key-safe
handling.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java`:
- Around line 122-126: 현재 상태를 무시하고 항상
ingestJob.updateIngestStatus(IngestStatus.FAILED) 및
media.updateMediaStatus(MediaStatus.FAILED)를 호출하고 있으니, IngestJobStatusManager에서
findIngestJob으로 가져온 IngestJob의 현재 상태를 확인해 PENDING 또는 PROCESSING일 때만 각각 FAILED로
전이하도록 변경하고(예: ingestJob.getStatus() / media.getStatus()), 이미 IngestStatus가
PARTIAL_SUCCESS 이상(또는 MediaStatus가 COMPLETED 이상)이면 상태를 변경하지 않고 실패 기록만 남기도록 분기
처리(로그 또는 실패 카운터 업데이트)하세요; 관련 식별자: IngestJob, findIngestJob,
ingestJob.updateIngestStatus, media.updateMediaStatus, IngestStatus,
MediaStatus.
In `@apps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.java`:
- Around line 62-64: The orchestration currently calls
statusManager.startProcessing(ingestJobId) but does not respect whether the job
is already in a terminal state, so handle()/the pipeline still runs for
SUCCESS/FAILED jobs; update behavior so startProcessing either returns a boolean
(e.g., true if moved to PROCESSING or already allowed, false if terminal) or add
an immediate status check in JobOrchestrator.handle(): call
statusManager.startProcessing(ingestJobId) and if it returns false (or if
current status is SUCCESS/FAILED), immediately return to skip the pipeline
(affecting the current call around statusManager.startProcessing and the
pipeline execution block referenced in JobOrchestrator.handle() / lines 96-117).
In
`@apps/transcoder/src/main/java/com/ott/transcoder/pipeline/hls/TranscodeCommandPipeline.java`:
- Line 34: The upload directory is built using
command.getCommandKey().toLowerCase(), causing inconsistency with logging and
the FFmpeg executor which use profile.resolution().getKey(); update the upload
path construction in TranscodeCommandPipeline (where TranscodeProfile profile =
transcodePlanner.plan(...) is used) to use
profile.resolution().getKey().toLowerCase(Locale.ROOT) instead of
command.getCommandKey().toLowerCase(), so all logging,
ProcessBuilderFfmpegExecutor and upload directory use the same canonical
lowercased resolution key.
---
Nitpick comments:
In `@modules/infra-mq/src/main/java/com/ott/infra/mq/TranscodeMessage.java`:
- Around line 14-20: TranscodeMessage's record fields mediaId and ingestJobId
must be non-null but are currently allowed to be null after Jackson
deserialization; add a compact constructor in the TranscodeMessage record that
validates mediaId and ingestJobId (e.g., via Objects.requireNonNull or explicit
checks) and throws a clear exception
(NullPointerException/IllegalArgumentException) with a descriptive message when
they are null so invalid messages fail fast during deserialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d5f2f0a-0ce5-4d61-90d7-c5872b68d2de
📒 Files selected for processing (15)
apps/transcoder/build.gradleapps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.javaapps/transcoder/src/main/java/com/ott/transcoder/config/RabbitConsumerConfig.javaapps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.javaapps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.javaapps/transcoder/src/main/java/com/ott/transcoder/pipeline/hls/TranscodeCommandPipeline.javaapps/transcoder/src/main/java/com/ott/transcoder/queue/MessageListener.javaapps/transcoder/src/main/java/com/ott/transcoder/queue/TranscodeMessage.javaapps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitDeadLetterListener.javaapps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitTranscodeListener.javamodules/infra-mq/build.gradlemodules/infra-mq/src/main/java/com/ott/infra/mq/TranscodeConstants.javamodules/infra-mq/src/main/java/com/ott/infra/mq/TranscodeMessage.javamodules/infra-mq/src/main/java/com/ott/infra/mq/config/MqMessageConfig.javasettings.gradle
💤 Files with no reviewable changes (1)
- apps/transcoder/src/main/java/com/ott/transcoder/queue/TranscodeMessage.java
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/transcoder/src/main/java/com/ott/transcoder/queue/rabbit/RabbitDeadLetterListener.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java (2)
40-51: LGTM - enum 비교는 == 사용 권장상태 전이 로직이 정확합니다. 선택적으로, Java enum 비교에서는
.equals()대신==를 사용하는 것이 관용적입니다 (null-safe하고 가독성이 좋음).♻️ enum 비교 개선 (선택)
- if (ingestJob.getIngestStatus().equals(IngestStatus.FAILED) || ingestJob.getIngestStatus().equals(IngestStatus.SUCCESS)) { + if (ingestJob.getIngestStatus() == IngestStatus.FAILED || ingestJob.getIngestStatus() == IngestStatus.SUCCESS) { return false; } - if (ingestJob.getIngestStatus().equals(IngestStatus.PENDING)) { + if (ingestJob.getIngestStatus() == IngestStatus.PENDING) { ingestJob.updateIngestStatus(IngestStatus.PROCESSING);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java` around lines 40 - 51, In startProcessing method of IngestJobStatusManager, replace enum comparisons using .equals() on ingestJob.getIngestStatus() with identity comparisons (==) against IngestStatus (e.g., IngestStatus.FAILED, SUCCESS, PENDING) to follow Java enum best practices; update the conditional checks that currently call .equals(...) to use "==" so comparisons remain null-safe and idiomatic when working with IngestJob.getIngestStatus() and when invoking ingestJob.updateIngestStatus(IngestStatus.PROCESSING).
69-86: LGTM!첫 트랜스코드 성공 시 미디어 활성화 로직이 정확합니다.
PROCESSING상태 확인으로 중복 활성화를 방지합니다.Line 72의 TODO 관련하여
IngestJobId + CommandKey유니크 키 도입이 필요하다면 구현을 도와드릴 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java` around lines 69 - 86, The TODO indicates adding a uniqueness constraint for IngestJobId + CommandKey to prevent duplicate command completions; implement this by updating the IngestCommand entity to include a CommandKey field (if missing) and annotate/create a unique constraint on (ingestJobId, commandKey), enforce it at DB schema level (migration) and optionally in the repository layer (e.g., findByIngestJobIdAndCommandKey) used by completeCommandInternal; also handle potential DataIntegrityViolationException in completeTranscodeCommand/completeCommandInternal to log/ignore duplicate inserts or translate them to a no-op so retries don’t double-activate media.apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormWriter.java (2)
143-196: updateShortFormUpload - EDITOR 권한 검증 로직
BackOfficeShortFormReader와 동일한 EDITOR 권한 검증 패턴이 사용되었습니다. Reader에서 제안한 대로 공통 유틸리티로 추출하면 Writer에서도 재사용할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormWriter.java` around lines 143 - 196, The editor-permission check in updateShortFormUpload duplicates the logic from BackOfficeShortFormReader; extract the validation into a shared utility method (e.g., BackOfficeAuthUtils.assertEditorOrOwner(Authentication auth, Long ownerId) or similar) that encapsulates checking Role.EDITOR and comparing ownerId with authentication.getPrincipal(), then replace the inline check in updateShortFormUpload (the block using isEditor, Role.EDITOR and media.getUploader().getId()) with a call to that utility; ensure the utility throws BusinessException(ErrorCode.FORBIDDEN) on failure so Writer reuses the same behavior as Reader.
67-79: placeholder 문자열 상수화 고려
"PENDING"문자열이posterUrl,thumbnailUrl,originUrl,masterPlaylistUrl에 반복 사용됩니다. 상수로 추출하면 오타 방지 및 일관성 유지에 도움이 됩니다.♻️ 리팩토링 제안
private static final String PENDING_URL = "PENDING";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormWriter.java` around lines 67 - 79, The repeated literal "PENDING" used for posterUrl and thumbnailUrl (and elsewhere such as originUrl and masterPlaylistUrl) should be extracted to a single constant to avoid typos and ensure consistency: add a private static final String PENDING_URL = "PENDING" in BackOfficeShortFormWriter and replace usages in Media.builder() (posterUrl, thumbnailUrl) and any other occurrences (originUrl, masterPlaylistUrl) to reference PENDING_URL instead of the string literal.apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormReader.java (1)
125-132: EDITOR 권한 검증 로직 중복
getShortFormDetail,getShortFormUploadInfo,getShortFormOriginUploadPartUrls메서드에서 동일한 EDITOR 권한 검증 패턴이 반복됩니다. 이 로직을 헬퍼 메서드로 추출하면 유지보수성이 향상됩니다.♻️ 리팩토링 제안
private void validateEditorAccess(Media media, Authentication authentication) { Long memberId = (Long) authentication.getPrincipal(); boolean isEditor = authentication.getAuthorities().stream() .anyMatch(authority -> Role.EDITOR.getKey().equals(authority.getAuthority())); if (isEditor && !media.getUploader().getId().equals(memberId)) { throw new BusinessException(ErrorCode.FORBIDDEN); } }Also applies to: 170-175, 194-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormReader.java` around lines 125 - 132, Extract the repeated EDITOR access check into a private helper in BackOfficeShortFormReader (e.g., validateEditorAccess(Media media, Authentication authentication)); move the logic that derives memberId, computes isEditor by checking Role.EDITOR, and throws new BusinessException(ErrorCode.FORBIDDEN) when editor != uploader into that helper, then replace the duplicated blocks in getShortFormDetail, getShortFormUploadInfo, and getShortFormOriginUploadPartUrls with calls to validateEditorAccess(media, authentication) to improve reuse and maintainability.apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java (1)
70-91: Phase 2~3 사이 실패 시 부분 완료 상태 가능S3 멀티파트 업로드 완료(Phase 2) 후 IngestJob 생성(Phase 3) 전에 예외가 발생하면, S3에는 파일이 완료되었지만 DB에는 IngestJob이 없는 상태가 됩니다. PR 목표에 명시된 "orphan resource management" TODO 항목과 관련됩니다.
현재 구조에서는 불가피한 트레이드오프이므로, 향후 orphan 정리 배치나 재시도 로직 구현 시 이 시나리오를 고려해주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java` around lines 70 - 91, completeShortFormOriginUpload can leave an orphaned S3 object if uploadHelper.completeMultipartUpload succeeds but writer.createIngestJob throws; catch exceptions around the IngestJob creation (reference completeShortFormOriginUpload, uploadHelper.completeMultipartUpload, writer.createIngestJob) and implement a compensating action: attempt to delete the completed S3 object (via an existing or new uploadHelper.deleteObject/deleteCompletedObject method) and/or enqueue a retry/cleanup task when deletion fails, then rethrow or translate the exception after logging; ensure logs include shortFormId/objectKey and failure context so the orphan cleanup batch or retry logic can find candidates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java`:
- Around line 105-117: The log message in checkAllCompleted() hardcodes
"PARTIAL_SUCCESS → SUCCESS" which can be inaccurate; before calling
ingestJob.updateIngestStatus(IngestStatus.SUCCESS) capture the current status
(e.g., via ingestJob.getIngestStatus()) and use that value in the log so it
reflects the actual transition (like "PROCESSING → SUCCESS" or "PARTIAL_SUCCESS
→ SUCCESS"); update the log.info call to print the captured previous status
instead of the fixed string and keep the rest of the flow (methods:
checkAllCompleted,
ingestCommandRepository.existsByIngestJobIdAndCommandStatusNot,
IngestJob.updateIngestStatus).
In `@apps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.java`:
- Around line 63-67: The condition in JobOrchestrator using
statusManager.startProcessing(ingestJobId) is inverted: startProcessing(...)
returns true when processing should proceed and false for already-terminated
jobs, so change the guard to return early when startProcessing(...) is false
(i.e., if (!statusManager.startProcessing(ingestJobId)) { log the "종료 상태
IngestJob 재수신" message and return; }) ensuring the log still refers to
terminated jobs and the normal processing path runs when startProcessing(...) is
true.
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormReader.java`:
- Around line 125-132: Extract the repeated EDITOR access check into a private
helper in BackOfficeShortFormReader (e.g., validateEditorAccess(Media media,
Authentication authentication)); move the logic that derives memberId, computes
isEditor by checking Role.EDITOR, and throws new
BusinessException(ErrorCode.FORBIDDEN) when editor != uploader into that helper,
then replace the duplicated blocks in getShortFormDetail,
getShortFormUploadInfo, and getShortFormOriginUploadPartUrls with calls to
validateEditorAccess(media, authentication) to improve reuse and
maintainability.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Around line 70-91: completeShortFormOriginUpload can leave an orphaned S3
object if uploadHelper.completeMultipartUpload succeeds but
writer.createIngestJob throws; catch exceptions around the IngestJob creation
(reference completeShortFormOriginUpload, uploadHelper.completeMultipartUpload,
writer.createIngestJob) and implement a compensating action: attempt to delete
the completed S3 object (via an existing or new
uploadHelper.deleteObject/deleteCompletedObject method) and/or enqueue a
retry/cleanup task when deletion fails, then rethrow or translate the exception
after logging; ensure logs include shortFormId/objectKey and failure context so
the orphan cleanup batch or retry logic can find candidates.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormWriter.java`:
- Around line 143-196: The editor-permission check in updateShortFormUpload
duplicates the logic from BackOfficeShortFormReader; extract the validation into
a shared utility method (e.g.,
BackOfficeAuthUtils.assertEditorOrOwner(Authentication auth, Long ownerId) or
similar) that encapsulates checking Role.EDITOR and comparing ownerId with
authentication.getPrincipal(), then replace the inline check in
updateShortFormUpload (the block using isEditor, Role.EDITOR and
media.getUploader().getId()) with a call to that utility; ensure the utility
throws BusinessException(ErrorCode.FORBIDDEN) on failure so Writer reuses the
same behavior as Reader.
- Around line 67-79: The repeated literal "PENDING" used for posterUrl and
thumbnailUrl (and elsewhere such as originUrl and masterPlaylistUrl) should be
extracted to a single constant to avoid typos and ensure consistency: add a
private static final String PENDING_URL = "PENDING" in BackOfficeShortFormWriter
and replace usages in Media.builder() (posterUrl, thumbnailUrl) and any other
occurrences (originUrl, masterPlaylistUrl) to reference PENDING_URL instead of
the string literal.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.java`:
- Around line 40-51: In startProcessing method of IngestJobStatusManager,
replace enum comparisons using .equals() on ingestJob.getIngestStatus() with
identity comparisons (==) against IngestStatus (e.g., IngestStatus.FAILED,
SUCCESS, PENDING) to follow Java enum best practices; update the conditional
checks that currently call .equals(...) to use "==" so comparisons remain
null-safe and idiomatic when working with IngestJob.getIngestStatus() and when
invoking ingestJob.updateIngestStatus(IngestStatus.PROCESSING).
- Around line 69-86: The TODO indicates adding a uniqueness constraint for
IngestJobId + CommandKey to prevent duplicate command completions; implement
this by updating the IngestCommand entity to include a CommandKey field (if
missing) and annotate/create a unique constraint on (ingestJobId, commandKey),
enforce it at DB schema level (migration) and optionally in the repository layer
(e.g., findByIngestJobIdAndCommandKey) used by completeCommandInternal; also
handle potential DataIntegrityViolationException in
completeTranscodeCommand/completeCommandInternal to log/ignore duplicate inserts
or translate them to a no-op so retries don’t double-activate media.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43f39f15-1424-43dc-a610-996b41e0fac2
📒 Files selected for processing (5)
apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormReader.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormWriter.javaapps/transcoder/src/main/java/com/ott/transcoder/job/IngestJobStatusManager.javaapps/transcoder/src/main/java/com/ott/transcoder/job/JobOrchestrator.java
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java (1)
77-78: 변수명이 코딩 가이드라인을 따르지 않습니다.
list변수명은 코딩 가이드라인의 컬렉션 변수List접미사 규칙을 따르지만, 의미가 불명확합니다.commandList또는transcodeCommandList로 변경하면 가독성이 향상됩니다.♻️ 변수명 개선 제안
private List<Command> buildCandidateList(TranscodeMessage message, ProbeResult probeResult) { - List<Command> list = new ArrayList<>(); + List<Command> commandList = new ArrayList<>(); for (Resolution resolution : Resolution.values()) { if (!probeResult.isUpscaleFor(resolution.getHeight())) { - list.add(new TranscodeCommand(resolution)); + commandList.add(new TranscodeCommand(resolution)); } } - if (list.isEmpty()) { + if (commandList.isEmpty()) { log.warn("모든 프로필 업스케일 → 360p fallback - mediaId: {}", message.mediaId()); - list.add(new TranscodeCommand(Resolution.P360)); + commandList.add(new TranscodeCommand(Resolution.P360)); } // TODO: 추가 커맨드 추출 - return list; + return commandList; }As per coding guidelines: "Collection variable names with
Listsuffix".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java` around lines 77 - 78, Rename the ambiguous local variable `list` in the method buildCandidateList(TranscodeMessage message, ProbeResult probeResult) to a clearer collection name (e.g., `commandList` or `transcodeCommandList`) and update all usages and references inside that method (including the return statement and any additions like add()/addAll()) to the new name; ensure the variable type remains List<Command> and no other methods or classes are affected.docker-compose.yml (1)
87-88:AI_TIMEOUT_MS하드코딩 고려다른 환경 변수들은
${VAR_NAME}패턴을 사용하는 반면,AI_TIMEOUT_MS는30000으로 하드코딩되어 있습니다. 환경별 유연성을 위해 기본값을 포함한 환경 변수 패턴 사용을 고려해주세요.♻️ 환경 변수 패턴으로 변경 제안
AI_BASE_URL: ${AI_BASE_URL} - AI_TIMEOUT_MS: 30000 + AI_TIMEOUT_MS: ${AI_TIMEOUT_MS:-30000}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 87 - 88, The AI_TIMEOUT_MS value is hardcoded to 30000; replace it with an environment variable pattern so it's configurable per environment while retaining the same default (e.g. use the shell-style expansion pattern for defaults) by changing the AI_TIMEOUT_MS entry to reference an env var with a fallback (keep default 30000) so services reading AI_TIMEOUT_MS can be overridden without code changes.apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java (1)
65-66: 컬렉션 파라미터 이름을List접미사로 맞추는 것을 권장합니다.
parts대신partsList(또는multipartPartETagList)처럼 타입 의미가 드러나게 맞추면 규칙 일관성이 좋아집니다.As per coding guidelines "Collection variable names with
Listsuffix".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` around lines 65 - 66, The parameter name parts in the method signature inside BackOfficeContentsService should follow the project's collection naming convention; rename it to partsList (or multipartPartETagList) to make the collection nature and element type UploadHelper.MultipartPartETag explicit, and update all usages (references, tests, javadoc) of parts accordingly to avoid compilation errors.apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java (1)
75-75: 컬렉션 파라미터 이름을List접미사로 통일해 주세요.
parts는 컬렉션 타입이므로partETagList같은 이름으로 맞추는 편이 가이드와 일치합니다.As per coding guidelines "Collection variable names with List suffix".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java` at line 75, The parameter name parts in the method signature (List<UploadHelper.MultipartPartETag> parts) should follow the collection naming guideline—rename it to partETagList (or multipartPartETagList) in the BackOfficeShortFormService method declaration and update all internal references within the method and any callers to use the new name; ensure the method signature change is compiled across the project (including unit tests and usages) so there are no unresolved references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 75-85: The current flow in BackOfficeContentsService creates an
ingest via writer.createIngestJob and then calls
transcodePublisher.publish(TranscodeMessage), which can leave the DB in a
committed PENDING/ALREADY_INGESTED state if publish fails; fix by implementing
an outbox or compensating state transition: persist the outbound
TranscodeMessage (an Outbox entry) within the same transaction as
writer.createIngestJob (so the DB and message are durable together) and have an
async dispatcher send messages from the outbox, or if you cannot add outbox now,
catch publish exceptions immediately after transcodePublisher.publish and update
the ingest record to a retryable/failure state (e.g., PENDING_PUBLISH_FAILED)
using the writer/update method so retries are allowed and ALREADY_INGESTED
checks do not permanently block reprocessing; reference writer.createIngestJob,
transcodePublisher.publish, TranscodeMessage and BackOfficeContentsService when
locating where to apply these changes.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/publish/RabbitTranscodePublisher.java`:
- Around line 17-22: Update RabbitTranscodePublisher.publish to guarantee
delivery: enable publisher confirms and returns in config (set
spring.rabbitmq.publisher-confirm-type: CORRELATED and
spring.rabbitmq.publisher-returns: true in application.yml), register a
ConfirmCallback and ReturnsCallback on the RabbitTemplate instance
(rabbitTemplate) used in RabbitTranscodePublisher, and in those callbacks handle
negative acks or returned messages for
TranscodeConstants.EXCHANGE_NAME/TranscodeConstants.ROUTING_KEY by logging
details and scheduling a retry or dead-lettering the TranscodeMessage; ensure
callbacks correlate confirms to the original message (use correlation data) so
publish() can react to ack/nack and returned routing failures.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Around line 83-94: The code currently commits the IngestJob in
BackOfficeShortFormWriter.createIngestJob and then calls
transcodePublisher.publish (RabbitTranscodePublisher.publish), which can throw
and cause lost transcode requests; as a minimal immediate fix, change the flow
so publish and DB write are atomic: either (A) move the publish call into the
same transactional boundary inside BackOfficeShortFormWriter.createIngestJob (or
add a transactional callback mechanism) so the DB commit occurs only after a
successful publish, or (B) if moving is not possible, wrap
transcodePublisher.publish with error handling that rolls back or deletes the
created IngestJob when publish fails (handle existsByMediaId idempotency), and
return a failure to the caller; for the long term implement a transactional
outbox pattern to persist the message with the DB write and publish
asynchronously from the outbox.
In `@apps/api-admin/src/main/resources/application.yml`:
- Around line 12-13: Remove the insecure defaults for RabbitMQ in
application.yml by eliminating the fallback values so the properties use the
environment variables directly (replace host: ${RABBITMQ_HOST:localhost} and
port: ${RABBITMQ_PORT:5672} with host: ${RABBITMQ_HOST} and port:
${RABBITMQ_PORT}) so Spring will fail when the env vars are missing; move the
local defaults into application-local.yml (define RABBITMQ_HOST/ RABBITMQ_PORT
with localhost:5672 there) and ensure any code/config that references these keys
(RABBITMQ_HOST, RABBITMQ_PORT, and the host/port properties) still reads from
the environment/profile-based properties.
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 65-66: The parameter name parts in the method signature inside
BackOfficeContentsService should follow the project's collection naming
convention; rename it to partsList (or multipartPartETagList) to make the
collection nature and element type UploadHelper.MultipartPartETag explicit, and
update all usages (references, tests, javadoc) of parts accordingly to avoid
compilation errors.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Line 75: The parameter name parts in the method signature
(List<UploadHelper.MultipartPartETag> parts) should follow the collection naming
guideline—rename it to partETagList (or multipartPartETagList) in the
BackOfficeShortFormService method declaration and update all internal references
within the method and any callers to use the new name; ensure the method
signature change is compiled across the project (including unit tests and
usages) so there are no unresolved references.
In
`@apps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.java`:
- Around line 77-78: Rename the ambiguous local variable `list` in the method
buildCandidateList(TranscodeMessage message, ProbeResult probeResult) to a
clearer collection name (e.g., `commandList` or `transcodeCommandList`) and
update all usages and references inside that method (including the return
statement and any additions like add()/addAll()) to the new name; ensure the
variable type remains List<Command> and no other methods or classes are
affected.
In `@docker-compose.yml`:
- Around line 87-88: The AI_TIMEOUT_MS value is hardcoded to 30000; replace it
with an environment variable pattern so it's configurable per environment while
retaining the same default (e.g. use the shell-style expansion pattern for
defaults) by changing the AI_TIMEOUT_MS entry to reference an env var with a
fallback (keep default 30000) so services reading AI_TIMEOUT_MS can be
overridden without code changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e57f0319-af9e-488b-a8a7-8c3384f1417e
📒 Files selected for processing (8)
apps/api-admin/build.gradleapps/api-admin/src/main/java/com/ott/api_admin/config/RabbitPublisherConfig.javaapps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.javaapps/api-admin/src/main/java/com/ott/api_admin/publish/RabbitTranscodePublisher.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javaapps/api-admin/src/main/resources/application.ymlapps/transcoder/src/main/java/com/ott/transcoder/command/CommandExtractor.javadocker-compose.yml
marulog
reviewed
Mar 15, 2026
Collaborator
marulog
left a comment
marulog
left a comment
There was a problem hiding this comment.
수고하셨습니다. (진짜 고생 많으셨다는거임..)
마지막 원본 업로드 시 메시지 발생쪽은 코드래빗이 잡아준거 같아서
따로 코멘트 직성안했습니다.
| if (command.getType() == CommandType.TRANSCODE) { | ||
| IngestJob ingestJob = findIngestJob(ingestJobId); | ||
| if (ingestJob.getIngestStatus() == IngestStatus.PROCESSING) { | ||
| ingestJob.updateIngestStatus(IngestStatus.PARTIAL_SUCCESS); |
Collaborator
There was a problem hiding this comment.
현재 순차처리라 문제는 없어 보이지만,
추후 병렬처리로 변경할 경우 특정 해상도의 경우에만 IngestJob, Media의 상태를 변경하도록 변경하면 더 좋을 것 같습니다!
| // TODO: IngestJobId & CommandKey Unique Key 도입 필요 | ||
| completeCommandInternal(ingestJobId, command, outputUrl); | ||
|
|
||
| // 2. 최초 트랜스코딩 성공 → 미디어 활성화 |
Collaborator
There was a problem hiding this comment.
순차 처리라 큰 문제는 없어보입니다
다만, 첫번째 트랜스코드 커맨드가 해당 메소드를 호출 할때 Job, media의 상태를 변경하는데 추후 병렬 처리 시 같은 id에 대하여 두번 업데이트 치는 경우를 추후 생각해봐야될 것 같습니다.
또한, 현재 트랜스코드 커맨드 추출 시 업스케일링을 막고 360p 커맨드를 필수로 생성하는데 이거를 이용하면 되지 않을까 생각이 들긴 합니다.
| /** | ||
| * 커맨드별 미디어 처리 파이프라인 | ||
| * 구현체는 미디어 처리 자체에만 집중 | ||
| * 구현체는 미디어 처리 + 산출물 업로드까지 담당 |
Collaborator
There was a problem hiding this comment.
추후 다양한 커맨드가 추가될 경우, 산출물이 없는 커맨드의 경우는
null값이나 블랭크로 처리하는건가요?
Contributor
Author
There was a problem hiding this comment.
네 맞아요 근데 웬만하면 커맨드는 산출물이 있을 것 같슴다
커맨드로 뺀다는 건 독립적이라는 것이고 그렇다면 본인의 무언가가 나온다는 것 일 수도 있기 때문입니다(아마도).
예를 들어, 스프라이트 시크바, 자막 이런 건 산출물이 나오고, DRM은 산출물이 따로 없습니다.
실제로 DRM은 트랜스코딩 파이프라인 내부에서 실행한다고 하네요!( 패키저 분리 )
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 작업 내용
📷 스크린샷
☑️ 체크 리스트
#️⃣ 연관된 이슈
closes #156
💬 리뷰 요구사항
상태 관련 정리해놓았습니다.
https://www.notion.so/3232753972b780e5b633f17998ef6a0d
1. Flyway V9 추가
### 2. 업로드 부분(백오피스) 미포함- 현재 이 PR은 컨슈머(트랜스코딩) 부분만 포함되어 있습니다.- => 포함해서 올리고 여기 부분 지움 처리 하겠습니다.3. 해야할 일
외에도 더 있을겁니다..
Summary by CodeRabbit
New Features
Improvements
Other