DanielCarter-stack commented on PR #10456:
URL: https://github.com/apache/seatunnel/pull/10456#issuecomment-3857709278
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10456", "part": 1,
"total": 1} -->
#### Issue 1: Missing edge case tests
**Location**: `CoordinatorServiceTest.java:341-358`
**Problem Description**:
The newly added test `testGetPendingJobInfo()` only covers the scenario
where "the job is in the pending queue". It lacks tests for "throwing
JobNotFoundException when the job does not exist". Although this is an
enhancement to existing code, the new exception behavior should be verified.
**Potential Risks**:
- Future code refactoring may accidentally break the exception logic
- Unable to verify through tests whether the error message for
`JobNotFoundException` is correct
**Impact Scope**:
- Direct impact: `CoordinatorServiceTest` test suite
- Indirect impact: Confidence during code refactoring
- Affected area: Core engine service layer
**Severity**: MINOR
**Improvement Suggestion**:
```java
@Test
void testGetJobInfoNotFound() {
CoordinatorService coordinatorService = getCoordinatorService();
long nonExistentJobId = 999999L;
JobNotFoundException exception = Assertions.assertThrows(
JobNotFoundException.class,
() -> coordinatorService.getJobInfo(nonExistentJobId)
);
Assertions.assertTrue(exception.getMessage().contains("999999"));
}
```
**Rationale**:
- Complete test coverage should include all branches
- Testing exception messages helps ensure error message debuggability
- Low cost, only requires adding a simple test method
---
#### Issue 2: Missing method documentation comments
**Location**: `CoordinatorService.java:867`
**Problem Description**:
The `getJobInfo()` method lacks JavaDoc comments, making it unclear what
order this method queries job information and what exceptions it might throw.
**Potential Risks**:
- New developers may not understand why three places need to be queried
- May be unclear under what circumstances `JobNotFoundException` is thrown
- IDE cannot provide accurate parameter and return value descriptions
**Impact Scope**:
- Direct impact: Maintainability of `CoordinatorService`
- Indirect impact: Callers' understanding of the method
- Affected area: Core engine service layer
**Severity**: MINOR
**Improvement Suggestion**:
```java
/**
* Get job information by job ID.
*
* <p>This method searches for job information in the following order:
* <ol>
* <li>Job history service (for finished jobs)</li>
* <li>Running job master map (for currently running jobs)</li>
* <li>Pending job queue (for jobs waiting for resources)</li>
* </ol>
*
* @param jobId the job ID to query
* @return the job DAG information
* @throws JobNotFoundException if the job is not found in any of the three
locations
*/
public JobDAGInfo getJobInfo(long jobId) {
// ... implementation
}
```
**Rationale**:
- Clear documentation helps team collaboration
- Explains the query order for better logic understanding
- Clarifies exception behavior, consistent with Java best practices
---
#### Issue 3: Missing debug logs
**Location**: `CoordinatorService.java:883`
**Problem Description**:
When a job does not exist, an exception is thrown directly without logging.
In production environments, if this error occurs frequently, the cause cannot
be traced.
**Potential Risks**:
- Unable to track the frequency of "job not found" events
- Lack of contextual information when debugging user issues
- Unable to monitor exception occurrence rates
**Impact Scope**:
- Direct impact: Production environment observability
- Indirect impact: Problem diagnosis efficiency
- Affected area: Production operations
**Severity**: MINOR
**Improvement Suggestion**:
```java
PendingJobInfo pendingJobInfo = pendingJobQueue.getById(jobId);
if (pendingJobInfo != null) {
return pendingJobInfo.getJobMaster().getJobDAGInfo();
}
logger.debug(String.format("Job %s not found in history, running, or pending
queue", jobId));
throw new JobNotFoundException(String.format("Job %s not found", jobId));
```
**Rationale**:
- Debug level logging does not affect performance
- Facilitates problem tracking in production environments
- Consistent with logging style elsewhere in the project
---
#### Issue 4: Query order inconsistency with getJobMaster()
**Location**:
- `CoordinatorService.java:392-398` (getJobMaster)
- `CoordinatorService.java:867-884` (getJobInfo)
**Problem Description**:
`getJobMaster()` queries `pendingJobQueue` first, then
`runningJobMasterMap`; whereas `getJobInfo()` queries `runningJobMasterMap`
first, then `pendingJobQueue`. Although this difference is reasonable under
their respective semantics, it may cause confusion.
**Potential Risks**:
- Developers may mistakenly assume the query logic of both methods is
consistent
- Future refactoring may inappropriately unify the logic, breaking existing
behavior
**Impact Scope**:
- Direct impact: Code maintainability
- Indirect impact: Safety of future refactoring
- Affected area: Core engine service layer
**Severity**: MINOR
**Improvement Suggestion**:
In the comments for `getJobMaster()` and `getJobInfo()`, explicitly explain
the different reasons for query order:
```java
/**
* Get the JobMaster for a job.
*
* <p>This method searches in the following order:
* <ol>
* <li>Pending job queue (for jobs waiting for resources)</li>
* <li>Running job master map (for currently running jobs)</li>
* </ol>
*
* <p>Note: The search order prioritizes pending jobs because this method is
typically
* used to obtain a JobMaster for job control operations, where pending jobs
should
* be handled first.
*/
public JobMaster getJobMaster(Long jobId) {
// ...
}
/**
* Get job information by job ID.
*
* <p>This method searches in the following order:
* <ol>
* <li>Job history service (for finished jobs)</li>
* <li>Running job master map (for currently running jobs)</li>
* <li>Pending job queue (for jobs waiting for resources)</li>
* </ol>
*
* <p>Note: The search order follows the job lifecycle (finished → running →
pending)
* to prioritize the most recent state for information queries.
*/
public JobDAGInfo getJobInfo(long jobId) {
// ...
}
```
**Rationale**:
- Explicitly explain design decisions to avoid future misunderstanding
- Explains why the query order differs between the two methods
- Improves code maintainability
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
