You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2020/07/23 11:11:06 UTC
[GitHub] [incubator-dolphinscheduler] RedemptionC opened a new pull request #3290: add feature: query top n process instance order by running duration
RedemptionC opened a new pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290
## What is the purpose of the pull request
complete feature(#3191)
## Brief change log
* add function in ProcessInstanceController.java , ProcessInstanceService.java , ProcessInstanceMapper.java
* add some test in ProcessInstanceServiceTest.java , ProcessInstanceMapperTest.java
* add some constant/status in Status.java , Constants.java
* add some sql in ProcessInstanceMapper.xml
## Verify this pull request
* Manually verified the change by testing locally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] RedemptionC commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
RedemptionC commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460091093
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ProcessInstanceController.java
##########
@@ -204,6 +204,28 @@ public Result queryProcessInstanceById(@ApiIgnore @RequestAttribute(value = Cons
return returnDataList(result);
}
+ @ApiOperation(value = "queryTopNLongestRunningProcessInstance", notes = "QUERY_TOPN_LONGEST_RUNNING_PROCESS_INSTANCE_NOTES")
+ @ApiImplicitParams({
+ @ApiImplicitParam(name = "size", value = "PROCESS_INSTANCE_SIZE", dataType = "Int", example = "10"),
+ @ApiImplicitParam(name = "startTime", value = "PROCESS_INSTANCE_START_TIME", dataType = "String"),
+ @ApiImplicitParam(name = "endTime", value = "PROCESS_INSTANCE_END_TIME", dataType = "String"),
+ })
+ @GetMapping(value = "/top-n")
+ @ResponseStatus(HttpStatus.OK)
+ @ApiException(QUERY_PROCESS_INSTANCE_BY_ID_ERROR)
+ public Result queryTopNLongestRunningProcessInstance(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
+ @ApiParam(name = "projectName", value = "PROJECT_NAME", required = true) @PathVariable String projectName,
+ @RequestParam("size") Integer size,
+ @RequestParam(value = "startTime",required = true) String startTime,
+ @RequestParam(value = "endTime",required = true) String endTime
+
+ ){
+ logger.info("query top {} SUCCESS process instance order by running time which started between startTime and endTime ,login user:{},project name:{}",size,
+ loginUser.getUserName(),projectName);
Review comment:
can you give me some advice?It seems other method log the same way
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460101457
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/ProcessInstanceService.java
##########
@@ -98,6 +98,50 @@
@Autowired
UsersService usersService;
+ /**
+ * return top n SUCCESS process instance order by running time which started between startTime and endTime
+ * @param loginUser
+ * @param projectName
+ * @param size
+ * @param startTime
+ * @param endTime
+ * @return
+ */
+ public Map<String, Object> queryTopNLongestRunningProcessInstance(User loginUser,String projectName,int size, String startTime, String endTime) {
+ Map<String, Object> result = new HashMap<>();
+
+ Project project = projectMapper.queryByName(projectName);
+ Map<String, Object> checkResult = projectService.checkProjectAndAuth(loginUser, project, projectName);
+ Status resultEnum = (Status) checkResult.get(Constants.STATUS);
+ if (resultEnum != Status.SUCCESS) {
+ return checkResult;
+ }
+
+ if (0 > size) {
+ putMsg(result, Status.NEGTIVE_SIZE_NUMBER_ERROR, size);
+ return result;
+ }
+ if (Objects.isNull(startTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.START_TIME);
+ return result;
+ }
+ Date start=DateUtils.stringToDate(startTime);
+ if (Objects.isNull(endTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.END_TIME);
+ return result;
+ }
+ // TODO the format really matters
+ Date end=DateUtils.stringToDate(endTime);
+ if (start.getTime() > end.getTime()) {
+ putMsg(result, Status.START_TIME_BIGGER_THAN_END_TIME_ERROR, startTime, endTime);
+ return result;
+ }
Review comment:
> I don't understand...
> and, I wonder why some unit test cause error while I never modify them?
1:These parameter verification should be judged before SQL execution.
2:Single test failure is not your problem, we will fix it later, if you are interested, you can also try to solve it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] RedemptionC closed pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
RedemptionC closed pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460100205
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/ProcessInstanceService.java
##########
@@ -98,6 +98,50 @@
@Autowired
UsersService usersService;
+ /**
+ * return top n SUCCESS process instance order by running time which started between startTime and endTime
+ * @param loginUser
+ * @param projectName
+ * @param size
+ * @param startTime
+ * @param endTime
+ * @return
+ */
+ public Map<String, Object> queryTopNLongestRunningProcessInstance(User loginUser,String projectName,int size, String startTime, String endTime) {
+ Map<String, Object> result = new HashMap<>();
+
+ Project project = projectMapper.queryByName(projectName);
+ Map<String, Object> checkResult = projectService.checkProjectAndAuth(loginUser, project, projectName);
+ Status resultEnum = (Status) checkResult.get(Constants.STATUS);
+ if (resultEnum != Status.SUCCESS) {
+ return checkResult;
+ }
+
+ if (0 > size) {
+ putMsg(result, Status.NEGTIVE_SIZE_NUMBER_ERROR, size);
+ return result;
+ }
+ if (Objects.isNull(startTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.START_TIME);
+ return result;
+ }
+ Date start=DateUtils.stringToDate(startTime);
+ if (Objects.isNull(endTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.END_TIME);
+ return result;
+ }
+ // TODO the format really matters
+ Date end=DateUtils.stringToDate(endTime);
+ if (start.getTime() > end.getTime()) {
+ putMsg(result, Status.START_TIME_BIGGER_THAN_END_TIME_ERROR, startTime, endTime);
+ return result;
+ }
Review comment:
Test failure is not your problem, we will fix it later, if you are interested, you can also try to solve it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] RedemptionC commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
RedemptionC commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460095527
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/ProcessInstanceService.java
##########
@@ -98,6 +98,50 @@
@Autowired
UsersService usersService;
+ /**
+ * return top n SUCCESS process instance order by running time which started between startTime and endTime
+ * @param loginUser
+ * @param projectName
+ * @param size
+ * @param startTime
+ * @param endTime
+ * @return
+ */
+ public Map<String, Object> queryTopNLongestRunningProcessInstance(User loginUser,String projectName,int size, String startTime, String endTime) {
+ Map<String, Object> result = new HashMap<>();
+
+ Project project = projectMapper.queryByName(projectName);
+ Map<String, Object> checkResult = projectService.checkProjectAndAuth(loginUser, project, projectName);
+ Status resultEnum = (Status) checkResult.get(Constants.STATUS);
+ if (resultEnum != Status.SUCCESS) {
+ return checkResult;
+ }
+
+ if (0 > size) {
+ putMsg(result, Status.NEGTIVE_SIZE_NUMBER_ERROR, size);
+ return result;
+ }
+ if (Objects.isNull(startTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.START_TIME);
+ return result;
+ }
+ Date start=DateUtils.stringToDate(startTime);
+ if (Objects.isNull(endTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.END_TIME);
+ return result;
+ }
+ // TODO the format really matters
+ Date end=DateUtils.stringToDate(endTime);
+ if (start.getTime() > end.getTime()) {
+ putMsg(result, Status.START_TIME_BIGGER_THAN_END_TIME_ERROR, startTime, endTime);
+ return result;
+ }
Review comment:
I don't understand...
and, I wonder why some unit test cause error while I never modify them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] RedemptionC commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
RedemptionC commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460594102
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ProcessInstanceController.java
##########
@@ -204,6 +204,28 @@ public Result queryProcessInstanceById(@ApiIgnore @RequestAttribute(value = Cons
return returnDataList(result);
}
+ @ApiOperation(value = "queryTopNLongestRunningProcessInstance", notes = "QUERY_TOPN_LONGEST_RUNNING_PROCESS_INSTANCE_NOTES")
+ @ApiImplicitParams({
+ @ApiImplicitParam(name = "size", value = "PROCESS_INSTANCE_SIZE", dataType = "Int", example = "10"),
+ @ApiImplicitParam(name = "startTime", value = "PROCESS_INSTANCE_START_TIME", dataType = "String"),
+ @ApiImplicitParam(name = "endTime", value = "PROCESS_INSTANCE_END_TIME", dataType = "String"),
+ })
+ @GetMapping(value = "/top-n")
+ @ResponseStatus(HttpStatus.OK)
+ @ApiException(QUERY_PROCESS_INSTANCE_BY_ID_ERROR)
+ public Result queryTopNLongestRunningProcessInstance(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
+ @ApiParam(name = "projectName", value = "PROJECT_NAME", required = true) @PathVariable String projectName,
+ @RequestParam("size") Integer size,
+ @RequestParam(value = "startTime",required = true) String startTime,
+ @RequestParam(value = "endTime",required = true) String endTime
+
+ ){
+ logger.info("query top {} SUCCESS process instance order by running time which started between startTime and endTime ,login user:{},project name:{}",size,
+ loginUser.getUserName(),projectName);
Review comment:
could you please
> In this case, this kind of log does not make much sense, on the other hand, it makes our log files huge.
could you please give me some advice on how to make the log meaningful?
I have found that I should replace the starttime and endtime with params
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460014589
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/ProcessInstanceService.java
##########
@@ -98,6 +98,50 @@
@Autowired
UsersService usersService;
+ /**
+ * return top n SUCCESS process instance order by running time which started between startTime and endTime
+ * @param loginUser
+ * @param projectName
+ * @param size
+ * @param startTime
+ * @param endTime
+ * @return
+ */
+ public Map<String, Object> queryTopNLongestRunningProcessInstance(User loginUser,String projectName,int size, String startTime, String endTime) {
+ Map<String, Object> result = new HashMap<>();
+
+ Project project = projectMapper.queryByName(projectName);
+ Map<String, Object> checkResult = projectService.checkProjectAndAuth(loginUser, project, projectName);
+ Status resultEnum = (Status) checkResult.get(Constants.STATUS);
+ if (resultEnum != Status.SUCCESS) {
+ return checkResult;
+ }
+
+ if (0 > size) {
+ putMsg(result, Status.NEGTIVE_SIZE_NUMBER_ERROR, size);
+ return result;
+ }
+ if (Objects.isNull(startTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.START_TIME);
+ return result;
+ }
+ Date start=DateUtils.stringToDate(startTime);
+ if (Objects.isNull(endTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.END_TIME);
+ return result;
+ }
+ // TODO the format really matters
+ Date end=DateUtils.stringToDate(endTime);
+ if (start.getTime() > end.getTime()) {
+ putMsg(result, Status.START_TIME_BIGGER_THAN_END_TIME_ERROR, startTime, endTime);
+ return result;
+ }
Review comment:
The verification should be advanced
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ProcessInstanceController.java
##########
@@ -204,6 +204,28 @@ public Result queryProcessInstanceById(@ApiIgnore @RequestAttribute(value = Cons
return returnDataList(result);
}
+ @ApiOperation(value = "queryTopNLongestRunningProcessInstance", notes = "QUERY_TOPN_LONGEST_RUNNING_PROCESS_INSTANCE_NOTES")
+ @ApiImplicitParams({
+ @ApiImplicitParam(name = "size", value = "PROCESS_INSTANCE_SIZE", dataType = "Int", example = "10"),
+ @ApiImplicitParam(name = "startTime", value = "PROCESS_INSTANCE_START_TIME", dataType = "String"),
+ @ApiImplicitParam(name = "endTime", value = "PROCESS_INSTANCE_END_TIME", dataType = "String"),
+ })
+ @GetMapping(value = "/top-n")
+ @ResponseStatus(HttpStatus.OK)
+ @ApiException(QUERY_PROCESS_INSTANCE_BY_ID_ERROR)
+ public Result queryTopNLongestRunningProcessInstance(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
+ @ApiParam(name = "projectName", value = "PROJECT_NAME", required = true) @PathVariable String projectName,
+ @RequestParam("size") Integer size,
+ @RequestParam(value = "startTime",required = true) String startTime,
+ @RequestParam(value = "endTime",required = true) String endTime
+
+ ){
+ logger.info("query top {} SUCCESS process instance order by running time which started between startTime and endTime ,login user:{},project name:{}",size,
+ loginUser.getUserName(),projectName);
Review comment:
good job, but I don't think this log is meaningful
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] RedemptionC commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
RedemptionC commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460594702
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/ProcessInstanceService.java
##########
@@ -98,6 +98,50 @@
@Autowired
UsersService usersService;
+ /**
+ * return top n SUCCESS process instance order by running time which started between startTime and endTime
+ * @param loginUser
+ * @param projectName
+ * @param size
+ * @param startTime
+ * @param endTime
+ * @return
+ */
+ public Map<String, Object> queryTopNLongestRunningProcessInstance(User loginUser,String projectName,int size, String startTime, String endTime) {
+ Map<String, Object> result = new HashMap<>();
+
+ Project project = projectMapper.queryByName(projectName);
+ Map<String, Object> checkResult = projectService.checkProjectAndAuth(loginUser, project, projectName);
+ Status resultEnum = (Status) checkResult.get(Constants.STATUS);
+ if (resultEnum != Status.SUCCESS) {
+ return checkResult;
+ }
+
+ if (0 > size) {
+ putMsg(result, Status.NEGTIVE_SIZE_NUMBER_ERROR, size);
+ return result;
+ }
+ if (Objects.isNull(startTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.START_TIME);
+ return result;
+ }
+ Date start=DateUtils.stringToDate(startTime);
+ if (Objects.isNull(endTime)) {
+ putMsg(result, Status.DATA_IS_NULL, Constants.END_TIME);
+ return result;
+ }
+ // TODO the format really matters
+ Date end=DateUtils.stringToDate(endTime);
+ if (start.getTime() > end.getTime()) {
+ putMsg(result, Status.START_TIME_BIGGER_THAN_END_TIME_ERROR, startTime, endTime);
+ return result;
+ }
Review comment:
which sql ? I did put the queryTopNProcessInstance query after the params verification
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on a change in pull request #3290: add feature: query top n process instance order by running duration
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on a change in pull request #3290:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/3290#discussion_r460105759
##########
File path: dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ProcessInstanceController.java
##########
@@ -204,6 +204,28 @@ public Result queryProcessInstanceById(@ApiIgnore @RequestAttribute(value = Cons
return returnDataList(result);
}
+ @ApiOperation(value = "queryTopNLongestRunningProcessInstance", notes = "QUERY_TOPN_LONGEST_RUNNING_PROCESS_INSTANCE_NOTES")
+ @ApiImplicitParams({
+ @ApiImplicitParam(name = "size", value = "PROCESS_INSTANCE_SIZE", dataType = "Int", example = "10"),
+ @ApiImplicitParam(name = "startTime", value = "PROCESS_INSTANCE_START_TIME", dataType = "String"),
+ @ApiImplicitParam(name = "endTime", value = "PROCESS_INSTANCE_END_TIME", dataType = "String"),
+ })
+ @GetMapping(value = "/top-n")
+ @ResponseStatus(HttpStatus.OK)
+ @ApiException(QUERY_PROCESS_INSTANCE_BY_ID_ERROR)
+ public Result queryTopNLongestRunningProcessInstance(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
+ @ApiParam(name = "projectName", value = "PROJECT_NAME", required = true) @PathVariable String projectName,
+ @RequestParam("size") Integer size,
+ @RequestParam(value = "startTime",required = true) String startTime,
+ @RequestParam(value = "endTime",required = true) String endTime
+
+ ){
+ logger.info("query top {} SUCCESS process instance order by running time which started between startTime and endTime ,login user:{},project name:{}",size,
+ loginUser.getUserName(),projectName);
Review comment:
In this case, this kind of log does not make much sense, on the other hand, it makes our log files huge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org