Copilot commented on code in PR #17745:
URL:
https://github.com/apache/dolphinscheduler/pull/17745#discussion_r2567444397
##########
dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/WorkflowInstanceMapper.java:
##########
@@ -138,6 +138,9 @@ int updateWorkflowInstanceState(
@Param("originState")
WorkflowExecutionStatus originState,
@Param("targetState")
WorkflowExecutionStatus targetState);
+ int forceUpdateWorkflowInstanceState(Integer workflowInstanceId,
Review Comment:
The `workflowInstanceId` parameter is missing the `@Param` annotation. All
MyBatis mapper method parameters in this codebase consistently use `@Param`
annotations, as seen in other methods like `updateWorkflowInstanceState`. Add
`@Param("workflowInstanceId")` to maintain consistency and ensure proper
MyBatis parameter mapping.
```java
int forceUpdateWorkflowInstanceState(@Param("workflowInstanceId") Integer
workflowInstanceId,
@Param("targetState")
WorkflowExecutionStatus targetState);
```
```suggestion
int forceUpdateWorkflowInstanceState(@Param("workflowInstanceId")
Integer workflowInstanceId,
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/graph/WorkflowGraph.java:
##########
@@ -46,12 +45,20 @@ public WorkflowGraph(List<WorkflowTaskRelation>
workflowTaskRelations, List<Task
this.predecessors = new HashMap<>();
this.successors = new HashMap<>();
- this.taskDefinitionMap = taskDefinitions
- .stream()
- .collect(Collectors.toMap(TaskDefinition::getName,
Function.identity()));
- this.taskDefinitionCodeMap = taskDefinitions
- .stream()
- .collect(Collectors.toMap(TaskDefinition::getCode,
Function.identity()));
+ this.taskDefinitionMap = new HashMap<>(taskDefinitions.size());
+ this.taskDefinitionCodeMap = new HashMap<>(taskDefinitions.size());
+ for (TaskDefinition taskDefinition : taskDefinitions) {
+ if (taskDefinitionMap.containsKey(taskDefinition.getName())) {
+ throw new IllegalArgumentException(
+ "Duplicate task name: " + taskDefinition.getName() + "
in the workflow");
+ }
+ taskDefinitionMap.put(taskDefinition.getName(), taskDefinition);
+ if (taskDefinitionCodeMap.containsKey(taskDefinition.getCode())) {
+ throw new IllegalArgumentException(
+ "Duplicate task code: " + taskDefinition.getCode() + "
in the workflow");
+ }
+ taskDefinitionCodeMap.put(taskDefinition.getCode(),
taskDefinition);
+ }
Review Comment:
The new duplicate detection logic for task names and codes lacks test
coverage. Consider adding test cases to verify that:
1. An `IllegalArgumentException` is thrown when duplicate task names are
provided
2. An `IllegalArgumentException` is thrown when duplicate task codes are
provided
This validation is important for data integrity and should be tested to
prevent regressions.
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/command/CommandEngine.java:
##########
@@ -189,8 +193,15 @@ private Void bootstrapError(Command command, Throwable
throwable) {
throwable);
return null;
}
- log.error("Failed bootstrap command {} ",
JSONUtils.toPrettyJsonString(command), throwable);
+
+ log.warn("Failed bootstrap command {} ",
JSONUtils.toPrettyJsonString(command), throwable);
+
+
workflowInstanceDao.forceUpdateWorkflowInstanceState(command.getWorkflowInstanceId(),
+ WorkflowExecutionStatus.FAILURE);
+ log.info("Set workflow instance {} state to FAILURE",
command.getWorkflowInstanceId());
+
commandService.moveToErrorCommand(command,
ExceptionUtils.getStackTrace(throwable));
+ log.info("Move command {} to error command table", command.getId());
Review Comment:
Potential race condition: the workflow instance state update (line 199-200)
and command deletion (line 203) are not within the same transaction. If the
application crashes between these operations, the workflow instance may be
marked as FAILURE but the command remains in the command table instead of being
moved to the error command table. Consider wrapping both operations in a single
transaction, or ensure the `moveToErrorCommand` method's transaction also
includes the workflow instance state update.
##########
dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/repository/impl/WorkflowInstanceDaoImpl.java:
##########
@@ -75,6 +75,11 @@ public void updateWorkflowInstanceState(Integer
workflowInstanceId, WorkflowExec
}
}
+ @Override
+ public void forceUpdateWorkflowInstanceState(Integer workflowInstanceId,
WorkflowExecutionStatus targetState) {
+ mybatisMapper.forceUpdateWorkflowInstanceState(workflowInstanceId,
targetState);
+ }
Review Comment:
The `forceUpdateWorkflowInstanceState` method lacks test coverage. Since
`WorkflowInstanceDaoImplTest` already exists with tests for
`updateWorkflowInstanceState` (lines 71-97), consider adding a test case for
this new method to verify it correctly updates the workflow instance state
without checking the original state, ensuring the "force update" behavior is
properly tested.
--
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]
