[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #12990: [Feature][API] New restful API for workflowInstance

[GitBox <gi...@apache.org>]

zhongjiajie commented on code in PR #12990:
URL: https://github.com/apache/dolphinscheduler/pull/12990#discussion_r1032056550


##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java:
##########
@@ -344,6 +360,35 @@ public Result queryProcessInstanceList(User loginUser, long projectCode, long pr
         return result;
     }
 
+    /**
+     * paging query process instance list, filtering according to project, process definition, time range, keyword, process status
+     *
+     * @param loginUser login user
+     * @param projectName project name
+     * @param pageNo page number
+     * @param pageSize page size
+     * @param processDefineName process definition name
+     * @param searchVal search value
+     * @param stateType state type
+     * @param host host
+     * @param startDate start time
+     * @param endDate end time
+     * @return process instance list
+     */
+    @Override
+    public Result queryProcessInstanceList(User loginUser, String projectName, String processDefineName,
+                                           String startDate, String endDate, String searchVal, String executorName,
+                                           WorkflowExecutionStatus stateType, String host, Integer pageNo,
+                                           Integer pageSize) {
+        Project project = projectMapper.queryByName(projectName);
+        ProcessDefinition processDefinition =
+                processDefineMapper.queryByDefineName(project.getCode(), processDefineName);
+
+        return queryProcessInstanceList(loginUser, project.getCode(), processDefinition.getCode(),

Review Comment:
   I do not think reusing the old method is a good idea, we should better query by `ProcessInstance` entity instead of reusing the old one, which you can find more detail in https://github.com/zhongjiajie/dolphinscheduler/blob/7d0e2cbbb9fcd2e1543f5166f0518eda03afae89/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessDefinitionServiceImpl.java#L632-L633



##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -83,7 +83,7 @@ ProcessInstance handleCommand(String host,
 
     void removeTaskLogFile(Integer processInstanceId);
 
-    void deleteWorkTaskInstanceByProcessInstanceId(int processInstanceId);
+    void deleteWorkTaskInstanceByProcessInstanceId(ProcessInstance processInstance);

Review Comment:
   should we also change this name? we not delete by id anymore



##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/dto/workflowInstance/WorkflowInstanceListPagingResponse.java:
##########
@@ -0,0 +1,20 @@
+package org.apache.dolphinscheduler.api.dto.workflowInstance;
+
+import org.apache.dolphinscheduler.api.utils.PageInfo;
+import org.apache.dolphinscheduler.api.utils.Result;
+import org.apache.dolphinscheduler.dao.entity.ProcessInstance;
+
+/**
+ * workflow instance list paging response
+ */
+public class WorkflowInstanceListPagingResponse extends Result {

Review Comment:
   we should directly use class Result instead of creating the new one, https://github.com/apache/dolphinscheduler/blob/af9374c2c201763ce4302f761389de18f24aa410/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/WorkflowV2Controller.java#L120-L126



##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java:
##########
@@ -766,6 +811,22 @@ public Map<String, Object> deleteProcessInstanceById(User loginUser, long projec
         return result;
     }
 
+    /**
+     * delete workflow instance by id, at the same time，delete task instance and their mapping relation data
+     *
+     * @param loginUser login user
+     * @param workflowInstanceId workflow instance id
+     * @return delete result code
+     */
+    @Override
+    public Map<String, Object> deleteProcessInstanceById(User loginUser, Integer workflowInstanceId) {
+        ProcessInstance processInstance = processInstanceMapper.selectById(workflowInstanceId);
+        ProcessDefinition processDefinition =

Review Comment:
   should we add an NPE catch in this object?



##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/dto/workflowInstance/WorkflowInstanceQueryRequest.java:
##########
@@ -0,0 +1,47 @@
+package org.apache.dolphinscheduler.api.dto.workflowInstance;
+
+import org.apache.dolphinscheduler.api.dto.PageQueryDto;
+import org.apache.dolphinscheduler.common.enums.TaskExecuteType;
+import org.apache.dolphinscheduler.common.enums.WorkflowExecutionStatus;
+
+import lombok.Data;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import io.swagger.v3.oas.annotations.media.Schema;
+
+/**
+ * workflow instance request
+ */
+@JsonIgnoreProperties(ignoreUnknown = true)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+@Data
+public class WorkflowInstanceQueryRequest extends PageQueryDto {

Review Comment:
   It should more likely to entity `ProcessInstance` instead of the old search param from the old one



-- 
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: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org