Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace TaskManager's runNow with runSoon #134324

Merged
merged 32 commits into from
Jun 28, 2022
Merged

Replace TaskManager's runNow with runSoon #134324

merged 32 commits into from
Jun 28, 2022

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Jun 14, 2022 •
edited
Loading

fixes: #133550

This PR intends to replace runNow method of task manager with runSoon.
runSoon gets the task and check if it's idle. If so, updates the task runAt and scheduledAt properties so it would be picked up by the Task Manager at the first run cycle.

runSoon uses pull method while runNow uses push (RxJS). Therefore i had to add some retry.try checks to the tests to be sure that the task is finished.

@ersin-erdal ersin-erdal added Feature:Task Manager Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.4.0 labels Jun 14, 2022
ersin-erdal
ersin-erdal commented Jun 23, 2022
View reviewed changes
src/plugins/dashboard/server/plugin.ts
plugins.taskManager.runNow(TASK_ID);
scheduleDashboardTelemetry(this.logger, plugins.taskManager)!.then(() => {
plugins.taskManager.runSoon(TASK_ID);
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we switch to pull model with runSoon, we have to wait the task to be created before trying to run it.

@ersin-erdal ersin-erdal marked this pull request as ready for review June 23, 2022 11:13
@ersin-erdal ersin-erdal requested review from a team as code owners June 23, 2022 11:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@ersin-erdal ersin-erdal added the release_note:skip Skip the PR/issue when compiling release notes label Jun 23, 2022
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 23, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

ymao1
ymao1 reviewed Jun 23, 2022
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review looks good so far! Left a comment about adding a .catch to handle errors and whether we should be trying to run failed tasks

x-pack/plugins/task_manager/server/task_scheduling.ts Outdated Show resolved Hide resolved
x-pack/plugins/task_manager/server/task_scheduling.ts Outdated Show resolved Hide resolved
x-pack/plugins/task_manager/server/task_scheduling.ts Outdated Show resolved Hide resolved
x-pack/plugins/task_manager/server/task_scheduling.ts Show resolved Hide resolved
x-pack/plugins/task_manager/server/task_scheduling.ts Show resolved Hide resolved
x-pack/test/plugin_api_integration/test_suites/task_manager/task_management.ts Show resolved Hide resolved
src/plugins/dashboard/server/usage/dashboard_telemetry_collection_task.ts Outdated Show resolved Hide resolved
src/plugins/dashboard/server/plugin.ts Outdated Show resolved Hide resolved
sorenlouv
sorenlouv approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APM changes lgtm

ThomThomson
ThomThomson approved these changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! Ran this locally and ensured that the dashboard telemetry task was run as scheduled. Code changes also look good!

ymao1
ymao1 reviewed Jun 27, 2022
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more minor comments about tests and doc and then it sounds like we should also be running failed tasks with runSoon :)

src/plugins/dashboard/server/plugin.test.ts Show resolved Hide resolved
x-pack/plugins/task_manager/README.md Outdated Show resolved Hide resolved
mikecote
mikecote approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM after Ying's comments!

ymao1
ymao1 approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for adding the tests!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #21 / endpoint endpoint list when there is data, "before all" hook for "finds page title"

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ersin-erdal ersin-erdal merged commit a529577 into elastic:main Jun 28, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 28, 2022
@ersin-erdal ersin-erdal deleted the 133550-replace-run-now branch June 28, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson approved these changes

@ymao1 ymao1 ymao1 approved these changes

@mikecote mikecote mikecote approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Task Manager release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace task manager's runNow with runSoon
8 participants
@ersin-erdal @elasticmachine @kibana-ci @sorenlouv @mikecote @ymao1 @ThomThomson @kibanamachine