-
Notifications
You must be signed in to change notification settings - Fork 590
Added query for composed-task-runner status. #5792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added query for composed-task-runner status. #5792
Conversation
#Fixes 5782
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, great job! Had some annoying questions ;-)
this.resourceAssembler = new TaskExecutionThinResourceAssembler(); | ||
} | ||
|
||
@GetMapping(produces = "application/json") | ||
@ResponseStatus(HttpStatus.OK) | ||
public PagedModel<TaskExecutionThinResource> listTasks(Pageable pageable, PagedResourcesAssembler<AggregateTaskExecution> pagedAssembler) { | ||
return pagedAssembler.toModel(explorer.findAll(pageable, true), resourceAssembler); | ||
Page<AggregateTaskExecution> page = explorer.findAll(pageable, true); | ||
taskJobService.populateComposeTaskRunnerStatus(page.getContent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we determined the cost of running the retrieval of the CTR status for the results set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I compared before and after timings from org.springframework.cloud.dataflow.integration.test.tasks.TaskExecutionQueryIT
I added index in parent_execution_id and now the difference is hardly noticeable.
execution.setCtrTaskStatus(ctrStatus); | ||
}); | ||
} | ||
LOG.info("updated {} ctr statuses", updated.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
|
||
@RequestMapping(value = "", method = RequestMethod.GET, params = "name") | ||
@ResponseStatus(HttpStatus.OK) | ||
public PagedModel<TaskExecutionThinResource> retrieveTasksByName(@RequestParam("name") String taskName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test to verify the population of the status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding
Added check for taskExecutionStatus on task execution list.
@@ -313,6 +313,10 @@ public void stopJobExecution(long jobExecutionId, String schemaTarget) throws No | |||
logger.info("stopped:{}:{}:status={}", jobExecutionId, schemaTarget, status); | |||
} | |||
|
|||
@Override | |||
public void populateComposeTaskRunnerStatus(Collection<AggregateTaskExecution> taskExecutions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to add a test here as well.
Fixes #5782