RAC-464 fix : batch JobParameter 수정 #325
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant Job
participant Logger
Scheduler->>Job: launchSalaryJob()
Job->>Scheduler: checkSalaryJobSuccess()
Scheduler-->>Scheduler: Create JobParameters
Scheduler->>Job: Execute salaryJobWithAdmin
alt Interrupted
Scheduler->>Logger: Log error
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/com/postgraduate/batch/scheduler/JobSchedulerConfig.java (2)
93-96: Consider implementing job instance cleanupCreating new JobParameters for each retry solves the duplicate parameter issue but could lead to multiple incomplete job instances in the job repository. Consider implementing cleanup logic for failed job instances.
+ // Add to class fields + @Autowired + private JobRepository jobRepository; // Add cleanup method + private void cleanupFailedJobInstances(String jobName) { + jobRepository.getJobInstances(jobName, 0, 10).stream() + .filter(instance -> { + JobExecution lastExecution = jobRepository.getLastJobExecution(instance); + return lastExecution != null && lastExecution.getStatus() == BatchStatus.FAILED; + }) + .forEach(instance -> log.info("Failed job instance found: {}", instance.getInstanceId())); + }
Line range hint
75-102: Enhance retry mechanism and error handlingThe current retry implementation has several areas for improvement:
- The sleep between retries might be unnecessary since we're creating new job instances
- Error handling could be more robust
- The retry count and delay could be configurable
Consider this enhanced implementation:
- private void checkSalaryJobSuccess() throws JobExecutionAlreadyRunningException, JobRestartException, JobInstanceAlreadyCompleteException, JobParametersInvalidException { + @Value("${batch.retry.max-attempts:5}") + private int maxRetries; + @Value("${batch.retry.delay:5000}") + private long retryDelay; + private void checkSalaryJobSuccess() { int retries = 0; boolean success = false; int seniorSize = seniorGetService.allSeniorId().size(); - while (retries < MAX_RETRIES){ + while (retries < maxRetries) { try { int salarySize = salaryGetService.findAllNext().size(); if (salarySize == seniorSize) { success = true; break; } + JobParameters jobParameters = new JobParametersBuilder() + .addString("attempt", String.valueOf(retries + 1)) + .addLocalDateTime("date", LocalDateTime.now()) + .toJobParameters(); + jobLauncher.run(salaryJobWithAdmin, jobParameters); Thread.sleep(retryDelay); - } catch (InterruptedException ex) { + } catch (Exception ex) { Thread.currentThread().interrupt(); - log.error("Thread Interrupt 발생"); + log.error("Error during salary job retry {}: {}", retries + 1, ex.getMessage()); + slackErrorMessage.sendSlackError("Salary Job Retry Failed", ex.getMessage()); } - JobParameters jobParameters = new JobParametersBuilder() - .addLocalDateTime("date", LocalDateTime.now()) - .toJobParameters(); - jobLauncher.run(salaryJobWithAdmin, jobParameters); retries++; } if (!success) { slackErrorMessage.sendSlackSalaryError(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/com/postgraduate/batch/scheduler/JobSchedulerConfig.java(2 hunks)
🔇 Additional comments (3)
src/main/java/com/postgraduate/batch/scheduler/JobSchedulerConfig.java (3)
72-72: Consistent with previous changes
The change follows the same pattern as in launchSalaryJob.
75-75: Method signature change improves parameter handling
The removal of JobParameters parameter aligns with the fix for duplicate parameter issues.
64-64: Verify the impact of discarding initial JobParameters
The initial JobParameters created in launchSalaryJob are no longer passed to checkSalaryJobSuccess. While this helps avoid the duplicate parameter issue, we should verify that important parameter values aren't being lost.
✅ Verification successful
Based on the search results, there are no instances of code accessing JobParameters values from the Spring Batch job execution. The search shows that the codebase primarily uses getValue, getString, etc. methods in other contexts like date formatting, Redis operations, and entity mapping, but not for accessing Spring Batch job parameters.
No impact from removing JobParameters parameter
The removal of JobParameters from checkSalaryJobSuccess method call appears safe as there's no evidence of the parameters being used for any critical functionality in the job execution. The change aligns with the PR's objective of fixing the duplicate parameter issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any code that might be relying on specific JobParameters values
rg -l "getJobParameters|getParameters" | xargs rg -A 5 "getValue|getString|getDouble|getLong|getDate"
Length of output: 29219
🦝 PR 요약
batch JobParameter 수정
✨ PR 상세 내용
�jobParameter 값 변경하는 방식으로 변경 -> 동일한 파라미터를 가지는 잡이 실행되기 때문에 두번째 시도부터 오류 발생
🚨 주의 사항
주의할 부분이 무엇인가요? - 지우고 작성
✅ 체크 리스트
Summary by CodeRabbit