Skip to content
This repository was archived by the owner on May 14, 2025. It is now read-only.

Added query for composed-task-runner status.#5792

Merged
corneil merged 4 commits intospring-attic:mainfrom
corneil:corneil/gh-5782-thin-execution-task-status
May 6, 2024
Merged

Added query for composed-task-runner status.#5792
corneil merged 4 commits intospring-attic:mainfrom
corneil:corneil/gh-5782-thin-execution-task-status

Conversation

@corneil
Copy link
Contributor

@corneil corneil commented May 2, 2024

Fixes #5782

@corneil corneil requested review from cppwfs and onobc May 2, 2024 14:06
Copy link
Contributor

@cppwfs cppwfs left a 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 ;-)

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());
Copy link
Contributor

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?

Copy link
Contributor Author

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());
Copy link
Contributor

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?

Copy link
Contributor Author

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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.
@corneil corneil requested a review from cppwfs May 3, 2024 12:01
}

@Override
public void populateComposeTaskRunnerStatus(Collection<AggregateTaskExecution> taskExecutions) {
Copy link
Contributor

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.

@corneil corneil requested a review from cppwfs May 3, 2024 13:12
@cppwfs cppwfs added this to the 2.11.3 milestone May 3, 2024
@corneil corneil merged commit 6f97589 into spring-attic:main May 6, 2024
@corneil corneil deleted the corneil/gh-5782-thin-execution-task-status branch May 6, 2024 08:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Fix the thin task execution page query for CTR status

2 participants